-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Turnkey Passkey POC #1481
Conversation
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 SwiftLint (0.57.0)ios/ConverseNotificationExtension/Xmtp/Messages.swift[ WalkthroughThis pull request introduces comprehensive support for passkey-based wallet connection and authentication in the Converse application. The changes span multiple files and modules, implementing a new onboarding and account creation flow using passkeys. Key additions include new screens for passkey onboarding, state management for passkey authentication, utility functions for creating and managing passkeys, and updates to navigation and configuration to support this new authentication method. Changes
Sequence DiagramsequenceDiagram
participant User
participant PasskeyScreen
participant PasskeyAuthStore
participant TurnkeyClient
participant XMTPClient
User->>PasskeyScreen: Initiate Passkey Creation
PasskeyScreen->>PasskeyAuthStore: Set Loading State
PasskeyScreen->>TurnkeyClient: Create Passkey
TurnkeyClient-->>PasskeyScreen: Return Authenticator Params
PasskeyScreen->>TurnkeyClient: Create Sub-Organization
TurnkeyClient-->>PasskeyScreen: Return Wallet Info
PasskeyScreen->>XMTPClient: Initialize Client
XMTPClient-->>PasskeyScreen: Client Ready
PasskeyScreen->>PasskeyAuthStore: Update Account State
PasskeyScreen->>User: Display Success
Possibly related PRs
Suggested Reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
async loadClient() { | ||
// return new LocalAccount(this.address); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loadClient
method appears to be a stub that returns undefined
. Since this is part of the wallet interface, it should either return a valid LocalAccount
instance or throw an appropriate error if the account cannot be loaded. Consider implementing the account loading logic or documenting why this method might intentionally be left unimplemented.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
async createClient() { | ||
const client = await onPasskeyCreate(); | ||
if (!client) { | ||
throw new Error("Failed to create client"); | ||
} | ||
return client; | ||
} | ||
|
||
async loadClient() { | ||
// return new LocalAccount(this.address); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method names createClient
and loadClient
don't match the interface contract defined in IEmbeddedWallet
. To properly implement the interface, these methods should be renamed to createAccount
and loadAccount
respectively. This will ensure type safety and maintain consistency with the interface definition.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 14
🧹 Nitpick comments (8)
utils/passkeys/createPasskey.ts (2)
44-44
: Use a more robust method for generatinguserId
Generating
userId
usingDate.now()
converted to base64 may lead to collisions if passkeys are created rapidly. Consider using a universally unique identifier (UUID) to ensure uniqueness.Update the code to generate a UUID:
- const userId = Buffer.from(String(Date.now())).toString("base64"); + import { v4 as uuidv4 } from 'uuid'; + const userId = Buffer.from(uuidv4()).toString("base64");Ensure to install and import the
uuid
package.
77-79
: Remove unnecessary null check forclient
The
client
variable is instantiated just before the null check, so it cannot benull
. The checkif (!client)
is unnecessary and can be removed to simplify the code.Apply this diff:
stamper ); - if (!client) { - throw new Error("No client found"); - }utils/xmtpRN/signer.ts (1)
21-21
: Consider extracting the chain ID comment to a constant.This comment is duplicated from the
ethersSignerToXmtpSigner
function. Consider extracting it to a constant to maintain DRY principles.+const CHAIN_ID_COMMENT = "// We don't really care about the chain id because we support https://eips.ethereum.org/EIPS/eip-6492"; + export const ethersSignerToXmtpSigner = ( signer: ethers.Signer, isSCW?: boolean ): XmtpSigner => ({ getAddress: () => signer.getAddress(), - getChainId: () => ethereum.id, // We don't really care about the chain id because we support https://eips.ethereum.org/EIPS/eip-6492 + getChainId: () => ethereum.id, ${CHAIN_ID_COMMENT} getBlockNumber: () => undefined, walletType: () => (isSCW ? "SCW" : "EOA"), signMessage: (message: string) => signer.signMessage(message), }); export const viemSignerToXmtpSigner = ( signer: LocalAccount<any> ): XmtpSigner => ({ getAddress: () => Promise.resolve(signer.address), - getChainId: () => ethereum.id, // We don't really care about the chain id because we support https://eips.ethereum.org/EIPS/eip-6492 + getChainId: () => ethereum.id, ${CHAIN_ID_COMMENT} getBlockNumber: () => undefined, walletType: () => "SCW", signMessage: (message: string) => signer.signMessage({ message }), });features/onboarding/passkey/passkeyAuthStore.tsx (2)
46-52
: Optimize selector usage.Consider memoizing the selector to prevent unnecessary re-renders.
+import { useMemo } from 'react'; export function usePasskeyAuthStoreContext<T>( selector: (state: IPasskeyAuthStoreState) => T ): T { const store = useContext(PasskeyAuthStoreContext); if (!store) throw new Error("Missing PasskeyAuthStore.Provider in the tree"); - return useStore(store, selector); + const memoizedSelector = useMemo(() => selector, [selector]); + return useStore(store, memoizedSelector); }
4-11
: Add JSDoc documentation for types.Consider adding documentation for the store props and state to improve maintainability.
+/** + * Props for the passkey authentication store + */ type IPasskeyAuthStoreProps = { loading: boolean; }; +/** + * State and actions for the passkey authentication store + */ type IPasskeyAuthStoreState = IPasskeyAuthStoreProps & { setLoading: (loading: boolean) => void; reset: () => void; };screens/Onboarding/OnboardingPasskeyScreen.tsx (2)
65-65
: Remove console.log statements.Debug logging should be removed from production code or replaced with proper logging utilities.
- console.log("dm", dm); + logger.debug("[Passkey] DM conversation created", { conversationId: dm.id });Also applies to: 92-92
104-106
: Improve error message presentation.Consider using a proper error component with user-friendly messages and potentially an icon.
- <Text preset="body" color="caution"> - {error} - </Text> + <ErrorMessage + icon="warning" + message={formatErrorMessage(error)} + onRetry={handleRetry} + />screens/Navigation/Navigation.tsx (1)
214-217
: Add navigation options for consistency.Other screens in the SignedOutNavigation group use shared navigation options. Consider applying the same options to maintain consistency.
<NativeStack.Screen name="OnboardingPasskey" component={OnboardingPasskeyScreen} + options={{ + ...authScreensSharedScreenOptions, + }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lock
is excluded by!**/*.lock
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (17)
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx
(1 hunks)features/embedded-wallets/embedded-wallet.interface.ts
(1 hunks)features/embedded-wallets/ephemeral-embedded-wallet.ts
(1 hunks)features/embedded-wallets/turnkey-embedded-wallet.ts
(1 hunks)features/onboarding/passkey/passkeyAuthStore.tsx
(1 hunks)i18n/translations/en.ts
(2 hunks)ios/Converse/Converse.entitlements
(1 hunks)navigation/OnboardingNavigator.tsx
(2 hunks)package.json
(2 hunks)screens/Main.tsx
(1 hunks)screens/Navigation/Navigation.tsx
(2 hunks)screens/Onboarding/OnboardingGetStartedScreen.tsx
(2 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx
(1 hunks)utils/api.ts
(3 hunks)utils/navigation.ts
(1 hunks)utils/passkeys/createPasskey.ts
(1 hunks)utils/xmtpRN/signer.ts
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- screens/Main.tsx
🧰 Additional context used
🪛 GitHub Check: tsc
features/embedded-wallets/ephemeral-embedded-wallet.ts
[failure] 30-30:
Property 'loadAccount' in type 'EphemeralEmbeddedWallet' is not assignable to the same property in base type 'IEmbeddedWallet'.
features/embedded-wallets/turnkey-embedded-wallet.ts
[failure] 13-13:
Class 'TurnkeyEmbeddedWallet' incorrectly implements interface 'IEmbeddedWallet'.
[failure] 20-20:
Expected 1 arguments, but got 0.
🪛 GitHub Actions: Typescript CI
features/embedded-wallets/ephemeral-embedded-wallet.ts
[error] 30-30: Property 'loadAccount' in type 'EphemeralEmbeddedWallet' is not assignable to the same property in base type 'IEmbeddedWallet'
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (12)
features/embedded-wallets/turnkey-embedded-wallet.ts (1)
13-13
:⚠️ Potential issueFix method and property names to match
IEmbeddedWallet
interfaceThe class
TurnkeyEmbeddedWallet
implementsIEmbeddedWallet
, but the method and property names do not match the interface:
- The interface defines
account
, but the class usesclient
.- The interface defines
createAccount
andloadAccount
, but the class usescreateClient
andloadClient
.This mismatch leads to TypeScript errors and breaks the interface contract.
Apply this diff to align the class with the interface:
export class TurnkeyEmbeddedWallet implements IEmbeddedWallet { - client: LocalAccount | null = null; + account: LocalAccount | null = null; - async createClient() { + async createAccount() { - async loadClient() { + async loadAccount() {Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: tsc
[failure] 13-13:
Class 'TurnkeyEmbeddedWallet' incorrectly implements interface 'IEmbeddedWallet'.features/embedded-wallets/embedded-wallet.interface.ts (1)
6-16
: InterfaceIEmbeddedWallet
is well-defined and consistentThe
IEmbeddedWallet
interface correctly defines the necessary properties and methods for an embedded wallet, ensuring consistency across implementations.ios/Converse/Converse.entitlements (1)
11-11
: LGTM! Verify domain ownership.The addition of
webcredentials:dev.converse.xyz
is correct for enabling passkey support. Ensure that the associated domain has the required Apple app site association file.✅ Verification successful
Verified: Domain ownership and configuration are correct
The domain
dev.converse.xyz
is properly configured with the Apple app site association file and includes the correct app identifier (Q6W2FLK8DM.com.converse.dev) matching your project's configuration. The webcredentials setup is complete and valid.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Apple app site association file exists and contains required webcredentials configuration curl -s https://dev.converse.xyz/.well-known/apple-app-site-association | jq '.webcredentials'Length of output: 183
Script:
#!/bin/bash # Check project configuration files for bundle identifier and team ID fd "project.pbxproj|Info.plist" ios/ --exec cat {} \; | grep -E "PRODUCT_BUNDLE_IDENTIFIER|DEVELOPMENT_TEAM"Length of output: 666
navigation/OnboardingNavigator.tsx (1)
14-14
: LGTM! Navigation setup for passkey screen is properly implemented.The screen is correctly integrated into the onboarding flow with appropriate type definitions and navigator configuration.
Also applies to: 19-19, 53-56
screens/Onboarding/OnboardingGetStartedScreen.tsx (1)
15-15
: LGTM! Passkey option is well integrated into the onboarding flow.The passkey authentication option is appropriately positioned as the primary authentication method in the UI.
Also applies to: 62-66
screens/Onboarding/OnboardingPasskeyScreen.tsx (1)
1-128
: PR title doesn't match implementation.The PR title suggests "Move everything to backend" but the changes implement frontend passkey authentication. Please update the PR title to accurately reflect the changes or clarify if backend changes are missing.
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx (1)
27-37
: Implementation follows established patterns!The function is well-structured, type-safe, and consistent with other similar functions in the file. Good use of internationalization for the title.
i18n/translations/en.ts (1)
11-11
: Well-structured translations!Translation keys follow conventions and messages are clear. All necessary strings for the passkey flow are included.
Also applies to: 55-59
utils/api.ts (1)
273-281
: Clean implementation of App Check headers!The function properly validates token presence and returns a typed headers object.
package.json (3)
174-174
: Review breaking changes in react-native-passkey v3.0.0.The major version bump (2.x → 3.x) may introduce breaking changes that need to be addressed across the codebase.
Run this script to check the changelog and usage:
79-81
: Verify Turnkey dependencies compatibility and stability.The new Turnkey dependencies are being added to support passkey authentication. Since these packages handle critical authentication functionality:
- Consider pinning exact versions instead of using caret ranges to prevent unexpected updates
- Ensure these versions are stable and production-ready
Run this script to check the stability and latest versions:
✅ Verification successful
Dependencies are suitable for production use
The Turnkey packages show consistent maintenance and stability:
- All packages receive regular updates with the latest releases on Dec 17, 2024
- Limited and focused integration in the codebase
- While @turnkey/viem is pre-1.0, it shows stable development patterns with consistent updates in the 0.6.x series
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the stability and latest versions of Turnkey packages # Check latest versions and release dates for pkg in "@turnkey/http" "@turnkey/react-native-passkey-stamper" "@turnkey/viem"; do echo "Checking $pkg..." curl -s "https://registry.npmjs.org/$pkg" | jq -r '. | "Latest version: \(.["dist-tags"].latest)\nPublished: \(.time[.["dist-tags"].latest])\n"' done # Check for known vulnerabilities yarn npm audit --json | jq '.advisories | select(. != null)'Length of output: 1818
Script:
#!/bin/bash # Check package popularity and development activity echo "Package download statistics (last week):" for pkg in "@turnkey/http" "@turnkey/react-native-passkey-stamper" "@turnkey/viem"; do echo "=== $pkg ===" curl -s "https://registry.npmjs.org/$pkg" | jq -r '. | "Downloads last week: \(.downloads)\nFirst published: \(.time.created)\nVersion history:\n\(.time | to_entries | sort_by(.value) | .[-5:] | map("\(.key): \(.value)") | .[])"' done # Check if these packages are used in other files echo -e "\nUsage in codebase:" rg --type js --type ts "@turnkey/(http|react-native-passkey-stamper|viem)" -lLength of output: 3240
Line range hint
79-174
: Verify dependency tree compatibility.The introduction of new Turnkey packages and the major update to react-native-passkey could introduce peer dependency conflicts.
Run this command to check for dependency conflicts:
✅ Verification successful
Dependencies are properly integrated and compatible ✅
The codebase shows correct integration of all Turnkey packages and react-native-passkey, with proper implementation patterns and no signs of compatibility issues. The packages are used together as intended in the passkey creation and authentication flows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for dependency conflicts and peer dependency issues # Check for peer dependency warnings yarn why @turnkey/http @turnkey/react-native-passkey-stamper @turnkey/viem react-native-passkey # Check for duplicate packages yarn list --pattern "@turnkey/*|react-native-passkey" --depth=0Length of output: 1375
Script:
#!/bin/bash # Check for lockfiles and package.json files echo "=== Checking for lock files ===" fd -t f "package-lock.json|yarn.lock" echo -e "\n=== Searching for peer dependencies in package.json ===" rg -A 5 '"peerDependencies"' --type json echo -e "\n=== Analyzing import patterns ===" rg -l "@turnkey|react-native-passkey" --type ts --type tsx --type js --type jsxLength of output: 485
Script:
#!/bin/bash # Search for imports and usage patterns with fixed file type handling echo "=== Searching for Turnkey imports ===" rg -l "@turnkey" --type-add 'typescript:*.{ts,tsx}' --type typescript --type javascript echo -e "\n=== Searching for react-native-passkey imports ===" rg -l "react-native-passkey" --type-add 'typescript:*.{ts,tsx}' --type typescript --type javascript echo -e "\n=== Looking for potential compatibility issues ===" rg -i "error.*(@turnkey|react-native-passkey)" --type-add 'typescript:*.{ts,tsx}' --type typescript --type javascriptLength of output: 731
Script:
#!/bin/bash # Simple search without file type specifications echo "=== Basic file search ===" # Search for any files containing the package names find . -type f -exec grep -l "@turnkey\|react-native-passkey" {} \; 2>/dev/null echo -e "\n=== Package versions ===" # Extract specific package versions from package.json grep -A 1 '"@turnkey\|"react-native-passkey"' package.jsonLength of output: 228
Script:
#!/bin/bash echo "=== Checking implementation details ===" # Check API implementation cat ./utils/api.ts echo -e "\n=== Checking passkey implementation ===" cat ./utils/passkeys/createPasskey.tsLength of output: 26976
async loadClient() { | ||
// return new LocalAccount(this.address); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement loadAccount
method or provide justification
The loadClient
method (should be loadAccount
after renaming) is currently empty and does not load an account. Since this method is part of the IEmbeddedWallet
interface, it should return a valid LocalAccount
instance or throw an appropriate error if the account cannot be loaded.
Consider implementing the account loading logic or explain why this method is intentionally left unimplemented.
const xmptClient = await Client.create( | ||
viemSignerToXmtpSigner(this.client), | ||
options | ||
); | ||
return xmptClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix typo in variable name and assign xmtpClient
property
The variable xmptClient
appears to be a typo; it should be xmtpClient
. Additionally, the instantiated XMTP client should be assigned to the xmtpClient
property to align with the interface and allow future access.
Apply this diff to fix the typo and assign the property:
- const xmptClient = await Client.create(
+ this.xmtpClient = await Client.create(
viemSignerToXmtpSigner(this.client),
options
);
- return xmptClient;
+ return this.xmtpClient;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const xmptClient = await Client.create( | |
viemSignerToXmtpSigner(this.client), | |
options | |
); | |
return xmptClient; | |
this.xmtpClient = await Client.create( | |
viemSignerToXmtpSigner(this.client), | |
options | |
); | |
return this.xmtpClient; |
constructor(public readonly address: string) {} | ||
|
||
async createClient() { | ||
const client = await onPasskeyCreate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass passkeyName
argument to onPasskeyCreate
The onPasskeyCreate
function requires a passkeyName
argument, but it is called without any arguments. This will result in an error.
Apply this diff to pass the required argument:
const client = await onPasskeyCreate();
+ const client = await onPasskeyCreate(this.address);
Provide an appropriate passkeyName
based on your application requirements.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const client = await onPasskeyCreate(); | |
const client = await onPasskeyCreate(this.address); |
🧰 Tools
🪛 GitHub Check: tsc
[failure] 20-20:
Expected 1 arguments, but got 0.
utils/passkeys/createPasskey.ts
Outdated
const subOrg = "TODO"; | ||
const address = "TODO"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace placeholder values subOrg
and address
with actual data
In onPasskeySignature()
, the variables subOrg
and address
are set to "TODO"
. These placeholders need to be replaced with actual sub-organization IDs and addresses for the function to work correctly.
Consider passing these values as parameters or retrieving them dynamically.
utils/passkeys/createPasskey.ts
Outdated
organizationId: "ORG_ID", | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid hardcoding organizationId
in getWhoami()
The organizationId
is hardcoded as "ORG_ID"
, which will not work in a real environment. Consider retrieving the organizationId
dynamically or passing it as a parameter.
Update the code to obtain the actual organizationId
:
organizationId: "ORG_ID",
+ // Replace "ORG_ID" with the actual organization ID
Committable suggestion skipped: line range outside the PR's diff.
@@ -14,7 +14,7 @@ export const setConverseNavigatorRef = ( | |||
ref: NavigationContainerRef<NavigationParamList> | null | |||
) => { | |||
if (converseNavigatorRef) { | |||
logger.error("[Navigation] Conversation navigator ref already set"); | |||
// logger.error("[Navigation] Conversation navigator ref already set"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reconsider commenting out error logging.
Removing this error log could make it harder to debug issues where the navigator reference is being set multiple times, which might indicate underlying problems in the navigation setup.
- // logger.error("[Navigation] Conversation navigator ref already set");
+ logger.error("[Navigation] Conversation navigator ref already set");
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// logger.error("[Navigation] Conversation navigator ref already set"); | |
logger.error("[Navigation] Conversation navigator ref already set"); |
options | ||
); | ||
const dm = await xmptClient.conversations.newConversation( | ||
"0x04B1E6971559f4AB1453392220d4f8173F41D7d9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove hardcoded wallet address.
The hardcoded wallet address should be moved to a configuration file or environment variable.
- "0x04B1E6971559f4AB1453392220d4f8173F41D7d9"
+ config.xmtp.defaultRecipientAddress
Also applies to: 90-90
const handleLoginWithPasskey = useCallback(async () => { | ||
const account = await onPasskeySignature(); | ||
if (!account) { | ||
return; | ||
} | ||
const dbDirectory = await getDbDirectory(); | ||
const dbEncryptionKey = await getDbEncryptionKey(); | ||
const options = { | ||
env: "dev", | ||
enableV3: true, | ||
dbDirectory, | ||
dbEncryptionKey, | ||
} as const; | ||
|
||
const xmptClient = await Client.build(account.address, options); | ||
const dm = await xmptClient.conversations.newConversation( | ||
"0x04B1E6971559f4AB1453392220d4f8173F41D7d9" | ||
); | ||
console.log("dm", dm); | ||
dm.send("Hello from persisted passkey embedded wallet"); | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add proper error handling to handleLoginWithPasskey.
The login function lacks try-catch blocks and loading state management, unlike the create function.
const handleLoginWithPasskey = useCallback(async () => {
+ try {
+ setLoading(true);
const account = await onPasskeySignature();
if (!account) {
+ setError("Failed to get passkey signature");
return;
}
const dbDirectory = await getDbDirectory();
const dbEncryptionKey = await getDbEncryptionKey();
const options = {
env: "dev",
enableV3: true,
dbDirectory,
dbEncryptionKey,
} as const;
const xmptClient = await Client.build(account.address, options);
const dm = await xmptClient.conversations.newConversation(
"0x04B1E6971559f4AB1453392220d4f8173F41D7d9"
);
console.log("dm", dm);
dm.send("Hello from persisted passkey embedded wallet");
+ } catch (e) {
+ setError(e instanceof Error ? e.message : "Unknown error");
+ } finally {
+ setLoading(false);
+ }
}, []);
Committable suggestion skipped: line range outside the PR's diff.
env: "dev", | ||
enableV3: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove hardcoded development configuration.
The environment and feature flags should be configurable based on the deployment environment rather than hardcoded.
- env: "dev",
- enableV3: true,
+ env: config.xmtp.environment,
+ enableV3: config.xmtp.enableV3,
Committable suggestion skipped: line range outside the PR's diff.
utils/api.ts
Outdated
export const createTurnkeySubOrganization = async ( | ||
authenticatorParams: TurnkeyAuthenticatorParams | ||
): Promise<{ | ||
subOrganizationId: string; | ||
walletId: string; | ||
address: string; | ||
}> => { | ||
const { data } = await api.post( | ||
"/api/wallet/create-wallet-turnkey", | ||
{ | ||
authenticatorParams, | ||
}, | ||
{ | ||
headers: await getAppCheckTokenHeaders(), | ||
} | ||
); | ||
return data; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error handling for wallet creation failures.
The function should handle potential errors during wallet creation and provide meaningful error messages.
export const createTurnkeySubOrganization = async (
authenticatorParams: TurnkeyAuthenticatorParams
): Promise<{
subOrganizationId: string;
walletId: string;
address: string;
}> => {
+ try {
const { data } = await api.post(
"/api/wallet/create-wallet-turnkey",
{
authenticatorParams,
},
{
headers: await getAppCheckTokenHeaders(),
}
);
return data;
+ } catch (error) {
+ logger.error("[API] Failed to create Turnkey wallet", error);
+ throw new Error(
+ error instanceof Error
+ ? error.message
+ : "Failed to create wallet. Please try again."
+ );
+ }
};
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 24
🔭 Outside diff range comments (1)
screens/NewAccount/NewAccountEphemeralScreen.tsx (1)
Line range hint
28-43
: Implement secure key handling and improve error feedbackThe current implementation has several security and UX concerns:
- Private key remains in memory
- Generic error handling
- Limited loading state feedback
Consider implementing:
const generateWallet = useCallback(async () => { setLoading(true); try { const signer = new Wallet(utils.randomPrivateKey()); + const address = await signer.getAddress(); await initXmtpClient({ signer, - address: await signer.getAddress(), + address, isEphemeral: true, }); + // Clear sensitive data from memory + signer.privateKey = null; if (isMissingConverseProfile()) { router.navigate("NewAccountUserProfile"); } else { router.navigate("Chats"); } } catch (error) { sentryTrackError(error); + Alert.alert( + "Account Creation Failed", + "Unable to create your account. Please try again." + ); } finally { setLoading(false); } }, [setLoading, router]);
🧹 Nitpick comments (13)
utils/passkeys/create-passkey.ts (2)
19-64
: RefactoronPasskeyCreate
for better maintainabilityThe
onPasskeyCreate
function handles multiple responsibilities, including UI updates, passkey creation, sub-organization creation, and Turnkey account creation. This makes the function lengthy and harder to read and maintain.Consider breaking down the function into smaller, focused functions. For example:
- Extract status updates into a separate utility function.
- Separate the logic for passkey creation, sub-organization creation, and account creation into individual functions.
- Handle errors closer to where they may occur to improve error tracing.
This modular approach will enhance readability and make unit testing more straightforward.
86-88
: Use template literals for string concatenationCurrently, string concatenation is used to build
name
anddisplayName
, which can be less readable.Improve readability by using template literals:
name: passkeyName + " " + humanReadableDate, displayName: passkeyName + " " + humanReadableDate, + name: `${passkeyName} ${humanReadableDate}`, + displayName: `${passkeyName} ${humanReadableDate}`,utils/passkeys/create-suborganization.ts (1)
1-1
: Remove unused import ofcreatePasskey
The
createPasskey
function is imported but not used in this file.Apply this diff to remove the unused import:
-import { createPasskey } from "@turnkey/react-native-passkey-stamper";
utils/passkeys/turnkey-clients.ts (1)
3-3
: Consider using a more robust state management solutionUsing a global mutable object for client management could lead to race conditions and memory leaks. Consider:
- Using a proper state management solution (e.g., Redux)
- Implementing a cleanup mechanism for stale clients
- Adding TTL (Time-To-Live) for cached clients
utils/passkeys/passkeys.interfaces.ts (2)
3-7
: Add JSDoc documentation for TurnkeyStoreInfoConsider adding comprehensive documentation for this type as it contains sensitive wallet information.
+/** + * Represents the essential information stored for a Turnkey wallet. + * @property subOrganizationId - The unique identifier for the sub-organization + * @property walletId - The unique identifier for the wallet + * @property address - The Ethereum address associated with the wallet + */ export type TurnkeyStoreInfo = { subOrganizationId: string; walletId: string; address: string; };
9-16
: Consider adding validation patterns for PasskeyStateThe state type could benefit from stricter constraints and validation patterns.
+/** + * Represents the current state of a passkey authentication process. + */ export type PasskeyState = { loading: boolean; error: string | null; statusString: string | null; account: LocalAccount | null; turnkeyInfo: TurnkeyStoreInfo | null; - previousPasskeyName: string | null; + previousPasskeyName: string | null; + // Add validation patterns + readonly validate: () => boolean; };utils/passkeys/create-turnkey-account.ts (1)
30-35
: Implement retry mechanism for account creationAdd a retry mechanism for the account creation API call to handle temporary failures.
+ const MAX_RETRIES = 3; + let retries = 0; + + while (retries < MAX_RETRIES) { + try { const account = await createAccount({ client, organizationId: subOrganizationId, signWith: address, ethereumAddress: address, }); + return account; + } catch (error) { + retries++; + if (retries === MAX_RETRIES) throw error; + await new Promise(resolve => setTimeout(resolve, 1000 * retries)); + } + } - return account;utils/passkeys/add-wallet-to-passkey.ts (1)
27-42
: Enhance wallet creation parametersThe current implementation has several areas for improvement:
- Wallet name uses only UUID without timestamp or meaningful prefix
- No retry logic for API calls
- Missing validation for account parameters
Consider implementing:
+ const MAX_RETRIES = 3; + const generateWalletName = () => `Wallet_${Date.now()}_${uuidv4()}`; + + const createWalletWithRetry = async (client, params, retries = 0) => { + try { + return await client.createWallet(params); + } catch (e) { + if (retries < MAX_RETRIES && isRetryableError(e)) { + await delay(Math.pow(2, retries) * 1000); + return createWalletWithRetry(client, params, retries + 1); + } + throw e; + } + }; const res = await createWalletWithRetry(client, { type: "ACTIVITY_TYPE_CREATE_WALLET", timestampMs: Date.now().toString(), organizationId: subOrgId, parameters: { - walletName: "Wallet " + uuidv4(), + walletName: generateWalletName(), accounts: [ { curve: "CURVE_SECP256K1", pathFormat: "PATH_FORMAT_BIP32", path: "m/44'/60'/0'/0/0", addressFormat: "ADDRESS_FORMAT_ETHEREUM", }, ], }, });components/Onboarding/init-xmtp-client.ts (1)
15-21
: Group related imports and use named exportsThe imports could be better organized for clarity and maintainability.
- import { - createXmtpClientFromSigner, - createXmtpClientFromViemAccount, - } from "../../utils/xmtpRN/signIn"; - import { getXmtpClient } from "../../utils/xmtpRN/sync"; - import { Signer } from "ethers"; - import { LocalAccount } from "viem/accounts"; + // External imports + import { Signer } from "ethers"; + import { LocalAccount } from "viem/accounts"; + + // Internal imports + import { getXmtpClient } from "../../utils/xmtpRN/sync"; + import { + createXmtpClientFromSigner, + createXmtpClientFromViemAccount, + } from "../../utils/xmtpRN/signIn";features/onboarding/passkey/passkeyAuthStore.tsx (2)
58-59
: Enhance error handling in setLoadingThe
setLoading
implementation clears errors when loading starts but not when loading ends. This could hide important error states.- setLoading: (loading) => - loading ? set({ loading, error: undefined }) : set({ loading: false }), + setLoading: (loading) => set({ loading }),
6-23
: Export type definitionsConsider exporting the type definitions to allow external components to properly type their props and state.
-type IPasskeyAuthStoreProps = { +export type IPasskeyAuthStoreProps = { // ... }; -type IPasskeyAuthStoreState = IPasskeyAuthStoreProps & { +export type IPasskeyAuthStoreState = IPasskeyAuthStoreProps & { // ... };screens/NewAccount/NewAccountPasskeyScreen.tsx (1)
173-203
: Extract common functionality to reduce code duplicationThe
onboardWithPasskey
method duplicates logic fromcreatePasskey
andcreateXmtpClientFromAccount
. Consider extracting common functionality.Consider creating a utility function to handle the common passkey creation and XMTP client initialization flow:
const handlePasskeyCreation = async (passkeyName: string) => { const account = await onPasskeyCreate({ passkeyName, setStatusString, setPreviousPasskeyName, setTurnkeyInfo, }); return account; };utils/xmtpRN/signIn.ts (1)
71-120
: Enhance error handling and logging.While the implementation is solid, consider these improvements:
- Add more descriptive error message for inbox ID mismatch
- Add debug logging for key operations like database operations
if (client.inboxId !== inboxId) { - throw new Error("Inbox ids don't match"); + throw new Error(`Inbox ID mismatch. Expected: ${inboxId}, Got: ${client.inboxId}`); } logger.debug("Instantiating client from signer"); +logger.debug(`Using temporary directory: ${tempDirectory}`); +logger.debug(`Processing inbox ID: ${inboxId}`); const installationValid = await isClientInstallationValid(client); if (!installationValid) { + logger.debug(`Installation invalid for inbox ID: ${inboxId}, cleaning up...`); await client.dropLocalDatabaseConnection(); await deleteLibXmtpDatabaseForInboxId(inboxId); onInstallationRevoked(); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
components/Onboarding/init-xmtp-client.ts
(2 hunks)features/conversation/conversation.nav.tsx
(1 hunks)features/embedded-wallets/turnkey-embedded-wallet.ts
(1 hunks)features/onboarding/passkey/passkeyAuthStore.tsx
(1 hunks)i18n/translations/en.ts
(2 hunks)navigation/OnboardingNavigator.tsx
(3 hunks)screens/Navigation/Navigation.tsx
(7 hunks)screens/NewAccount/NewAccountEphemeralScreen.tsx
(1 hunks)screens/NewAccount/NewAccountPasskeyScreen.tsx
(1 hunks)screens/NewAccount/NewAccountScreen.tsx
(2 hunks)screens/Onboarding/OnboardingEphemeralScreen.tsx
(1 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx
(1 hunks)utils/passkeys/add-wallet-to-passkey.ts
(1 hunks)utils/passkeys/create-passkey.ts
(1 hunks)utils/passkeys/create-suborganization.ts
(1 hunks)utils/passkeys/create-turnkey-account.ts
(1 hunks)utils/passkeys/load-client-from-passkey.ts
(1 hunks)utils/passkeys/passkeys.constants.ts
(1 hunks)utils/passkeys/passkeys.interfaces.ts
(1 hunks)utils/passkeys/persist-passkeys.ts
(1 hunks)utils/passkeys/turnkey-clients.ts
(1 hunks)utils/xmtpRN/signIn.ts
(2 hunks)utils/xmtpRN/signer.ts
(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- screens/Onboarding/OnboardingEphemeralScreen.tsx
- features/conversation/conversation.nav.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- screens/Navigation/Navigation.tsx
- navigation/OnboardingNavigator.tsx
🧰 Additional context used
🪛 GitHub Check: tsc
features/embedded-wallets/turnkey-embedded-wallet.ts
[failure] 11-11:
'"@/utils/xmtpRN/signer"' has no exported member named 'viemSignerToXmtpSigner'. Did you mean 'ethersSignerToXmtpSigner'?
[failure] 13-13:
Class 'TurnkeyEmbeddedWallet' incorrectly implements interface 'IEmbeddedWallet'.
[failure] 20-20:
Expected 1 arguments, but got 0.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: performance-test
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (9)
screens/NewAccount/NewAccountScreen.tsx (2)
11-11
: LGTM! Clean import addition.The new import is properly grouped with related wallet connection item functions.
47-51
: LGTM! Well-integrated passkey option.The passkey connection option is cleanly implemented and follows the established patterns for wallet connection items. Positioning it as the first option in the Converse Account section suggests this is the preferred connection method.
Let's verify the navigation route exists:
✅ Verification successful
Navigation route "NewAccountPasskey" is properly configured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the "NewAccountPasskey" route is properly defined # Search for route definition rg -A 5 "NewAccountPasskey" --type typescriptLength of output: 80
Script:
#!/bin/bash # Search for NewAccountPasskey route and screen definitions rg -i "NewAccountPasskey" -A 5 -B 2 # Search for potential screen component file fd "NewAccountPasskey.*\.(tsx|ts|js|jsx)$"Length of output: 2903
utils/passkeys/passkeys.constants.ts (1)
1-1
: ConfirmRPID
value for production environmentThe
RPID
is set to"dev.converse.xyz"
, which may not be appropriate for production.Ensure that
RPID
matches the relying party ID for the production environment. If different between environments, consider externalizing it to a configuration file or environment variable.utils/xmtpRN/signer.ts (1)
17-25
: Add isSCW parameter for wallet type consistencyThe
viemAccountToXmtpSigner
function hardcodes the wallet type to "SCW", which is inconsistent with theethersSignerToXmtpSigner
implementation.export const viemAccountToXmtpSigner = ( - account: LocalAccount<any> + account: LocalAccount<any>, + isSCW?: boolean ): XmtpSigner => ({ getAddress: () => Promise.resolve(account.address), getChainId: () => ethereum.id, getBlockNumber: () => undefined, - walletType: () => "SCW", + walletType: () => (isSCW ? "SCW" : "EOA"), signMessage: (message: string) => account.signMessage({ message }), });features/embedded-wallets/turnkey-embedded-wallet.ts (2)
19-25
: Pass required passkeyName parameter to onPasskeyCreateThe
createClient
method is missing the requiredpasskeyName
parameter.async createClient() { - const client = await onPasskeyCreate(); + const client = await onPasskeyCreate({ + passkeyName: this.address, + setStatusString: () => {}, + setPreviousPasskeyName: () => {}, + setTurnkeyInfo: () => {}, + }); if (!client) { throw new Error("Failed to create client"); } return client; }🧰 Tools
🪛 GitHub Check: tsc
[failure] 20-20:
Expected 1 arguments, but got 0.
46-50
: Fix typo in variable name and store xmtpClient instanceThe variable has a typo (
xmptClient
instead ofxmtpClient
) and the client instance isn't stored in the class property.- const xmptClient = await Client.create( + this.xmtpClient = await Client.create( viemSignerToXmtpSigner(this.client), options ); - return xmptClient; + return this.xmtpClient;utils/xmtpRN/signIn.ts (1)
12-13
: LGTM! Imports are properly organized.The imports for viem and signer utilities are correctly added.
screens/Onboarding/OnboardingPasskeyScreen.tsx (1)
69-70
: Consider potential memory leak with useRef.The inputTextRef is used to store text input value but doesn't get cleaned up.
Consider if this should be handled in a useEffect cleanup:
const inputTextRef = useRef<string>(""); +useEffect(() => { + return () => { + inputTextRef.current = ""; + }; +}, []);i18n/translations/en.ts (1)
11-11
: LGTM! Translation key properly added.The new passkey connection option is correctly added to the converseAccount section.
utils/passkeys/create-passkey.ts
Outdated
try { | ||
setStatusString("Creating passkey..."); | ||
|
||
const { authenticatorParams, userId } = | ||
await createUserPasskey(passkeyName); | ||
|
||
setPreviousPasskeyName(passkeyName); | ||
setStatusString("Passkey creation successful. Creating sub-org..."); | ||
|
||
const { walletId, subOrganizationId, address } = | ||
await createSubOrganization(authenticatorParams); | ||
savePasskeyInfo({ | ||
userId, | ||
walletId, | ||
subOrganizationId, | ||
address, | ||
passkeyName, | ||
}); | ||
if (!address) { | ||
throw new Error("No address found"); | ||
} | ||
|
||
setTurnkeyInfo({ | ||
walletId, | ||
subOrganizationId, | ||
address, | ||
}); | ||
setStatusString("Sub-org created! Creating turnkey account..."); | ||
|
||
const turnkeyAccount = await createTurnkeyAccount( | ||
address, | ||
subOrganizationId! | ||
); | ||
setStatusString(`Turnkey account created! Address is ${address}`); | ||
return turnkeyAccount; | ||
} catch (e) { | ||
console.error("error during passkey creation", e); | ||
throw e; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling and user feedback
Currently, the try...catch
block catches errors but only logs them and rethrows. Users might not receive meaningful feedback in case of failures.
- Provide user-friendly error messages by updating
setStatusString
with error details. - Handle specific errors differently, if applicable, to guide the user on next steps.
- Ensure that rethrown errors are appropriately handled upstream to prevent unhandled promise rejections.
utils/passkeys/create-passkey.ts
Outdated
const authenticatorParams = await createPasskey({ | ||
// This doesn't matter much, it will be the name of the authenticator persisted on the Turnkey side. | ||
// Won't be visible by default. | ||
authenticatorName: "End-User Passkey", | ||
rp: { | ||
id: RPID, | ||
name: "Passkey App", | ||
}, | ||
user: { | ||
id: userId, | ||
// ...but name and display names are | ||
// We insert a human-readable date time for ease of use | ||
name: passkeyName + " " + humanReadableDate, | ||
displayName: passkeyName + " " + humanReadableDate, | ||
}, | ||
authenticatorSelection: { | ||
residentKey: "required", | ||
requireResidentKey: true, | ||
userVerification: "preferred", | ||
}, | ||
}); | ||
return { authenticatorParams, userId }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error handling for createPasskey
call
The createPasskey
function may fail (e.g., due to user denial or system errors), but exceptions are not currently handled.
Wrap the createPasskey
call in a try...catch
block to handle potential errors:
let authenticatorParams;
try {
authenticatorParams = await createPasskey({ /* parameters */ });
} catch (error) {
alert("Failed to create passkey: " + error.message);
throw error;
}
This ensures that errors are communicated to the user, and the calling function can handle them appropriately.
const createUserPasskey = async (passkeyName: string) => { | ||
if (!isSupported()) { | ||
alert("Passkeys are not supported on this device"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent execution when passkeys are unsupported
In createUserPasskey
, if passkeys are not supported, an alert is shown, but the function continues execution, which could lead to unexpected behavior.
Apply this diff to exit the function when passkeys are unsupported:
if (!isSupported()) {
alert("Passkeys are not supported on this device");
+ return;
}
Alternatively, you may throw an error to be caught by the calling function.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const createUserPasskey = async (passkeyName: string) => { | |
if (!isSupported()) { | |
alert("Passkeys are not supported on this device"); | |
} | |
const createUserPasskey = async (passkeyName: string) => { | |
if (!isSupported()) { | |
alert("Passkeys are not supported on this device"); | |
return; | |
} |
const res = await createTurnkeySubOrganization(authenticatorParams); | ||
return res; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error handling for createTurnkeySubOrganization
The API call to createTurnkeySubOrganization
may fail, but there's no error handling in place.
Wrap the API call in a try...catch
block to handle potential errors and provide feedback:
try {
const res = await createTurnkeySubOrganization(authenticatorParams);
return res;
} catch (error) {
console.error("Failed to create sub-organization", error);
throw error;
}
This ensures that errors are logged and can be handled by the calling function.
export const setTurnkeyClient = ( | ||
subOrgId: string, | ||
turnkeyClient: TurnkeyClient | ||
) => { | ||
turnkeyClientMap[subOrgId] = turnkeyClient; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add input validation for setTurnkeyClient
The setter should validate its inputs to prevent potential issues.
export const setTurnkeyClient = (
subOrgId: string,
turnkeyClient: TurnkeyClient
) => {
+ if (!subOrgId?.trim()) {
+ throw new Error('subOrgId is required');
+ }
+ if (!turnkeyClient) {
+ throw new Error('turnkeyClient is required');
+ }
turnkeyClientMap[subOrgId] = turnkeyClient;
};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export const setTurnkeyClient = ( | |
subOrgId: string, | |
turnkeyClient: TurnkeyClient | |
) => { | |
turnkeyClientMap[subOrgId] = turnkeyClient; | |
}; | |
export const setTurnkeyClient = ( | |
subOrgId: string, | |
turnkeyClient: TurnkeyClient | |
) => { | |
if (!subOrgId?.trim()) { | |
throw new Error('subOrgId is required'); | |
} | |
if (!turnkeyClient) { | |
throw new Error('turnkeyClient is required'); | |
} | |
turnkeyClientMap[subOrgId] = turnkeyClient; | |
}; |
<TextField | ||
placeholder="Enter your passkey" | ||
onChangeText={(text) => { | ||
inputTextRef.current = text; | ||
}} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add input validation for passkey name
The TextField for passkey name lacks validation and error handling.
<TextField
placeholder="Enter your passkey"
+ error={inputTextRef.current?.length < 3 ? "Passkey name must be at least 3 characters" : undefined}
onChangeText={(text) => {
inputTextRef.current = text;
}}
+ onBlur={() => {
+ if (!inputTextRef.current) {
+ setError("Passkey name is required");
+ }
+ }}
/>
Committable suggestion skipped: line range outside the PR's diff.
const createXmtpClientFromAccount = useCallback(async () => { | ||
try { | ||
setLoading(true); | ||
if (!account) { | ||
setStatusString("Need to set an account first"); | ||
return; | ||
} | ||
await initXmtpClientFromViemAccount({ | ||
account, | ||
address: account.address, | ||
}); | ||
setStatusString("Xmtp client created"); | ||
if (isMissingConverseProfile()) { | ||
router.navigate("NewAccountUserProfile"); | ||
} else { | ||
router.navigate("Chats"); | ||
} | ||
} catch (err) { | ||
console.log("error creating Xmtp client", err); | ||
setStatusString(""); | ||
setError( | ||
"Error creating Xmtp client : " + | ||
(err instanceof Error ? err.message : "") + | ||
(typeof err === "string" ? err : "") | ||
); | ||
} finally { | ||
setLoading(false); | ||
} | ||
}, [account, router, setError, setLoading, setStatusString]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling in createXmtpClientFromAccount
The error handling concatenates different error types unsafely and doesn't properly clean up loading state in all paths.
} catch (err) {
- console.log("error creating Xmtp client", err);
+ console.error("Error creating Xmtp client:", err);
setStatusString("");
setError(
- "Error creating Xmtp client : " +
- (err instanceof Error ? err.message : "") +
- (typeof err === "string" ? err : "")
+ `Error creating Xmtp client: ${
+ err instanceof Error ? err.message :
+ typeof err === "string" ? err :
+ "Unknown error"
+ }`
);
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const createXmtpClientFromAccount = useCallback(async () => { | |
try { | |
setLoading(true); | |
if (!account) { | |
setStatusString("Need to set an account first"); | |
return; | |
} | |
await initXmtpClientFromViemAccount({ | |
account, | |
address: account.address, | |
}); | |
setStatusString("Xmtp client created"); | |
if (isMissingConverseProfile()) { | |
router.navigate("NewAccountUserProfile"); | |
} else { | |
router.navigate("Chats"); | |
} | |
} catch (err) { | |
console.log("error creating Xmtp client", err); | |
setStatusString(""); | |
setError( | |
"Error creating Xmtp client : " + | |
(err instanceof Error ? err.message : "") + | |
(typeof err === "string" ? err : "") | |
); | |
} finally { | |
setLoading(false); | |
} | |
}, [account, router, setError, setLoading, setStatusString]); | |
const createXmtpClientFromAccount = useCallback(async () => { | |
try { | |
setLoading(true); | |
if (!account) { | |
setStatusString("Need to set an account first"); | |
return; | |
} | |
await initXmtpClientFromViemAccount({ | |
account, | |
address: account.address, | |
}); | |
setStatusString("Xmtp client created"); | |
if (isMissingConverseProfile()) { | |
router.navigate("NewAccountUserProfile"); | |
} else { | |
router.navigate("Chats"); | |
} | |
} catch (err) { | |
console.error("Error creating Xmtp client:", err); | |
setStatusString(""); | |
setError( | |
`Error creating Xmtp client: ${ | |
err instanceof Error ? err.message : | |
typeof err === "string" ? err : | |
"Unknown error" | |
}` | |
); | |
} finally { | |
setLoading(false); | |
} | |
}, [account, router, setError, setLoading, setStatusString]); |
const createXmtpClientFromAccount = useCallback(async () => { | ||
try { | ||
setLoading(true); | ||
if (!account) { | ||
setStatusString("Need to set an account first"); | ||
return; | ||
} | ||
await initXmtpClientFromViemAccount({ | ||
account, | ||
address: account.address, | ||
}); | ||
if (isMissingConverseProfile()) { | ||
router.navigate("OnboardingUserProfile"); | ||
} else if (needToShowNotificationsPermissions()) { | ||
router.navigate("OnboardingNotifications"); | ||
} else { | ||
setAuthStatus("signedIn"); | ||
} | ||
setStatusString("Xmtp client created"); | ||
} catch (err) { | ||
console.log("error creating Xmtp client", err); | ||
setStatusString(""); | ||
setError( | ||
"Error creating Xmtp client : " + | ||
(err instanceof Error ? err.message : "") + | ||
(typeof err === "string" ? err : "") | ||
); | ||
} finally { | ||
setLoading(false); | ||
} | ||
}, [account, router, setError, setLoading, setStatusString]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling in createXmtpClientFromAccount.
The error handling could be improved:
- Console.log should use logger utility for consistency
- Error message concatenation could be simplified
- Status string is cleared on error which might confuse users
} catch (err) {
- console.log("error creating Xmtp client", err);
+ logger.error("Error creating Xmtp client:", err);
setStatusString("");
setError(
- "Error creating Xmtp client : " +
- (err instanceof Error ? err.message : "") +
- (typeof err === "string" ? err : "")
+ `Error creating Xmtp client: ${
+ err instanceof Error ? err.message : typeof err === "string" ? err : "Unknown error"
+ }`
);
}
Committable suggestion skipped: line range outside the PR's diff.
const handleCreatePasskey = useCallback(async () => { | ||
try { | ||
setLoading(true); | ||
const account = await onPasskeyCreate({ | ||
passkeyName: inputTextRef.current ?? "", | ||
setStatusString, | ||
setPreviousPasskeyName, | ||
setTurnkeyInfo, | ||
}); | ||
if (!account) { | ||
setError("No account created from Passkey"); | ||
return; | ||
} | ||
setAccount(account); | ||
} catch (e) { | ||
setError(e instanceof Error ? e.message : "Unknown error"); | ||
} finally { | ||
setLoading(false); | ||
} | ||
}, [ | ||
setAccount, | ||
setError, | ||
setLoading, | ||
setPreviousPasskeyName, | ||
setStatusString, | ||
setTurnkeyInfo, | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Prevent multiple simultaneous operations.
The loading state is shared across all operations which could lead to race conditions.
Consider using operation-specific loading states:
+const [createLoading, setCreateLoading] = useState(false);
+const [loginLoading, setLoginLoading] = useState(false);
const handleCreatePasskey = useCallback(async () => {
try {
- setLoading(true);
+ setCreateLoading(true);
// ... rest of the code
} finally {
- setLoading(false);
+ setCreateLoading(false);
}
}, []);
Committable suggestion skipped: line range outside the PR's diff.
passkey: { | ||
title: "Create Passkey", | ||
add_account_title: "Add account by passkey", | ||
subtitle: "Create a passkey to connect to Converse", | ||
createButton: "Create Passkey", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add missing error messages for passkey operations.
The passkey translations should include error messages for common failure scenarios.
Add these translations:
passkey: {
title: "Create Passkey",
add_account_title: "Add account by passkey",
subtitle: "Create a passkey to connect to Converse",
createButton: "Create Passkey",
+ errors: {
+ creation_failed: "Failed to create passkey",
+ login_failed: "Failed to login with passkey",
+ invalid_name: "Invalid passkey name",
+ already_exists: "A passkey with this name already exists",
+ },
},
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
passkey: { | |
title: "Create Passkey", | |
add_account_title: "Add account by passkey", | |
subtitle: "Create a passkey to connect to Converse", | |
createButton: "Create Passkey", | |
}, | |
passkey: { | |
title: "Create Passkey", | |
add_account_title: "Add account by passkey", | |
subtitle: "Create a passkey to connect to Converse", | |
createButton: "Create Passkey", | |
errors: { | |
creation_failed: "Failed to create passkey", | |
login_failed: "Failed to login with passkey", | |
invalid_name: "Invalid passkey name", | |
already_exists: "A passkey with this name already exists", | |
}, | |
}, |
|
||
async instantiateXmtpClient() { | ||
if (!this.client) { | ||
throw new Error("Client not found"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels wrong if we’re extension in a client why are we throwing? Is the clients not found?
We shouldn’t expect a client if we’re in instantiating I think we just need to remove this bam or add one more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn’t proof of concept related, but I do not like the pattern of unnecessarily putting exhaust stand into context. Not sure why we would do this.
Are there any benefits?
e09bf3d
to
98fc4fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
screens/NewAccount/NewAccountConnectWalletScreen.tsx (1)
Line range hint
26-28
: Add missing dependency to useCallbackThe empty dependency array causes
isMissingConverseProfile
result to be stale, which could lead to incorrect navigation decisions.}, - // eslint-disable-next-line react-hooks/exhaustive-deps - [] + [router] );screens/NewAccount/NewAccountPrivateKeyScreen.tsx (1)
Line range hint
28-31
: Add private key format validationThe current implementation only checks if the private key is empty. Consider adding proper format validation to prevent runtime errors.
try { const trimmedPrivateKey = privateKey.trim(); if (!trimmedPrivateKey) return; + // Private keys are typically 64 characters (32 bytes) in hex format + if (!/^[0-9a-fA-F]{64}$/.test(trimmedPrivateKey)) { + throw new Error('Invalid private key format'); + } await loginWithPrivateKey(trimmedPrivateKey);
♻️ Duplicate comments (12)
utils/passkeys/create-suborganization.ts (1)
4-9
: 🛠️ Refactor suggestionAdd error handling and improve type safety.
The function needs error handling and would benefit from explicit return type definition.
+type SubOrganizationResponse = { + subOrganizationId: string; + walletId: string; + address: string; +}; + export async function createSubOrganization( authenticatorParams: Awaited<ReturnType<typeof createPasskey>> -) { - const res = await createTurnkeySubOrganization(authenticatorParams); +): Promise<SubOrganizationResponse> { + try { + const res = await createTurnkeySubOrganization(authenticatorParams); + return res; + } catch (error) { + console.error("Failed to create sub-organization", error); + throw error; + } +}utils/api.ts (1)
830-847
: 🛠️ Refactor suggestionAdd error handling for wallet creation.
The function should handle potential errors during wallet creation and provide meaningful error messages.
export const createTurnkeySubOrganization = async ( authenticatorParams: TurnkeyAuthenticatorParams ): Promise<{ subOrganizationId: string; walletId: string; address: string; }> => { + try { const { data } = await api.post( "/api/wallet/create-wallet-turnkey", { authenticatorParams, }, { headers: await getAppCheckTokenHeaders(), } ); return data; + } catch (error) { + logger.error("[API] Failed to create Turnkey wallet", error); + throw new Error( + error instanceof Error + ? error.message + : "Failed to create wallet. Please try again." + ); + } };utils/xmtpRN/signer.ts (1)
17-25
: 🛠️ Refactor suggestionAdd
isSCW
parameter to maintain consistency with ethers implementationThe
viemAccountToXmtpSigner
function assumes all Viem LocalAccounts are Smart Contract Wallets (SCW), which is inconsistent with theethersSignerToXmtpSigner
implementation that supports both SCW and EOA wallet types through anisSCW
parameter.Apply this diff to maintain consistency:
-export const viemAccountToXmtpSigner = ( - account: LocalAccount<any> -): XmtpSigner => ({ +export const viemAccountToXmtpSigner = ( + account: LocalAccount<any>, + isSCW?: boolean +): XmtpSigner => ({ getAddress: () => Promise.resolve(account.address), getChainId: () => ethereum.id, getBlockNumber: () => undefined, - walletType: () => "SCW", + walletType: () => (isSCW ? "SCW" : "EOA"), signMessage: (message: string) => account.signMessage({ message }), });components/Onboarding/init-xmtp-client.ts (1)
56-87
: 🛠️ Refactor suggestionReduce code duplication and improve error handling
The
initXmtpClientFromViemAccount
function largely duplicatesinitXmtpClient
. Consider refactoring to reduce duplication and improve error handling.utils/passkeys/add-wallet-to-passkey.ts (1)
13-25
: 🛠️ Refactor suggestionMove API URL to environment configuration
The hardcoded API URL should be moved to environment configuration.
Add input validation for subOrgId
The function should validate the
subOrgId
parameter before proceeding with the wallet creation.export const addWalletToPasskey = async ({ subOrgId, setStatusString, }: AddWalletToPasskeyParams) => { + if (!subOrgId?.trim()) { + throw new Error("Invalid subOrgId provided"); + } setStatusString("Adding wallet to passkey. Creating Passkey Stamper...");utils/passkeys/load-client-from-passkey.ts (3)
21-25
:⚠️ Potential issueCritical: Replace hardcoded challenge with secure random value.
The hardcoded "TODO" challenge compromises the security of the passkey authentication process. A static challenge makes the system vulnerable to replay attacks.
27-30
:⚠️ Potential issueAdd input validation for passkey response.
The passkey response is used without validation, which could lead to security vulnerabilities.
44-47
: 🛠️ Refactor suggestionEnhance error handling to prevent information leakage.
The current error handling logs the entire error object, which might expose sensitive information.
screens/Onboarding/OnboardingPasskeyScreen.tsx (3)
71-97
: 🛠️ Refactor suggestionPrevent multiple simultaneous operations.
The loading state is shared across all operations which could lead to race conditions.
147-177
: 🛠️ Refactor suggestionImprove error handling in createXmtpClientFromAccount.
The error handling could be improved for better user experience and security.
248-253
:⚠️ Potential issueAdd input validation for passkey name.
The passkey name input lacks validation which could lead to security issues.
i18n/translations/en.ts (1)
55-60
:⚠️ Potential issueAdd missing error messages for passkey operations.
The passkey translations should include error messages for common failure scenarios.
Add these translations:
passkey: { title: "Create Passkey", add_account_title: "Add account by passkey", subtitle: "Create a passkey to connect to Converse", createButton: "Create Passkey", + errors: { + creation_failed: "Failed to create passkey", + login_failed: "Failed to login with passkey", + invalid_name: "Invalid passkey name", + already_exists: "A passkey with this name already exists", + }, },
🧹 Nitpick comments (9)
utils/passkeys/create-suborganization.ts (1)
4-9
: Add JSDoc documentation.Add documentation to explain the purpose of the function and its parameters.
+/** + * Creates a new sub-organization using Turnkey's passkey authentication. + * @param authenticatorParams - Parameters from the passkey creation process + * @returns Promise resolving to the created sub-organization details + * @throws Error if sub-organization creation fails + */ export async function createSubOrganization(utils/api.ts (1)
273-281
: Enhance error message for App Check Token.The error message should provide more context about why the token might be unavailable and potential remediation steps.
if (!appCheckToken) { - throw new Error("No App Check Token Available"); + throw new Error( + "No App Check Token Available. This may indicate an issue with Firebase App Check initialization or device integrity verification." + ); }screens/NewAccount/NewAccountConnectWalletScreen.tsx (1)
Line range hint
30-35
: Add error tracking in error handlerConsider tracking errors consistently across screens using
sentryTrackError
.const handleErrorConnecting = useCallback( (arg: { error: Error }) => { + sentryTrackError(arg.error); router.goBack(); }, - // eslint-disable-next-line react-hooks/exhaustive-deps - [] + [router] );screens/NewAccount/NewAccountUserProfileScreen.tsx (1)
Line range hint
23-23
: Navigation pattern improvement across onboarding flowThe consistent change from
navigate
topopTo
across all screens is a good architectural decision that:
- Prevents navigation stack buildup during onboarding
- Ensures clean navigation state after completing onboarding
- Improves memory management by clearing unnecessary screens from the stack
This change aligns well with the PR objective of moving functionality to the backend, as it helps maintain a clean frontend state.
Also applies to: 36-36, 34-34, 45-45
config.ts (1)
79-79
: Consider consolidating domain configurations.The newly added
appDomain
values match the existingwebsiteDomain
values in each environment. Consider either:
- Using
websiteDomain
instead of introducing a new property, or- Document the specific purpose of
appDomain
vswebsiteDomain
if they serve different functions.Also applies to: 99-99, 134-134
features/embedded-wallets/embedded-wallet.interface.ts (1)
6-16
: Consider adding wallet status methodsFor better integration with passkey functionality, consider adding methods to check wallet and passkey status:
isInitialized(): boolean
- Check if wallet is properly initializedhasPasskey(): Promise<boolean>
- Check if passkey exists for this walletgetPasskeyStatus(): Promise<PasskeyStatus>
- Get detailed passkey statusThis would help with proper state management and user flow control in the UI.
utils/passkeys/add-wallet-to-passkey.ts (1)
27-42
: Extract wallet creation parameters to constantsThe wallet creation parameters contain several magic strings that should be extracted to named constants for better maintainability and reusability.
+const WALLET_PARAMS = { + CURVE: "CURVE_SECP256K1", + PATH_FORMAT: "PATH_FORMAT_BIP32", + DEFAULT_PATH: "m/44'/60'/0'/0/0", + ADDRESS_FORMAT: "ADDRESS_FORMAT_ETHEREUM" +} as const; + const res = await client.createWallet({ type: "ACTIVITY_TYPE_CREATE_WALLET", timestampMs: Date.now().toString(), organizationId: subOrgId, parameters: { walletName: "Wallet " + uuidv4(), accounts: [ { - curve: "CURVE_SECP256K1", - pathFormat: "PATH_FORMAT_BIP32", - path: "m/44'/60'/0'/0/0", - addressFormat: "ADDRESS_FORMAT_ETHEREUM", + curve: WALLET_PARAMS.CURVE, + pathFormat: WALLET_PARAMS.PATH_FORMAT, + path: WALLET_PARAMS.DEFAULT_PATH, + addressFormat: WALLET_PARAMS.ADDRESS_FORMAT, }, ], }, });features/onboarding/passkey/passkeyAuthStore.tsx (1)
46-68
: Consider adding state persistence.The store implementation could benefit from state persistence to handle page reloads and app restarts gracefully.
Consider using Zustand's persist middleware:
+import { persist } from 'zustand/middleware' const createPasskeyAuthStore = (initProps: IPasskeyAuthStoreProps) => { const DEFAULT_PROPS: IPasskeyAuthStoreProps = { // ... }; - return createStore<IPasskeyAuthStoreState>()((set) => ({ + return createStore<IPasskeyAuthStoreState>()( + persist( + (set) => ({ // ... existing store code - })); + }), + { + name: 'passkey-auth-storage', + partialize: (state) => ({ + previousPasskeyName: state.previousPasskeyName, + turnkeyInfo: state.turnkeyInfo, + }), + } + ) + ); };screens/Onboarding/OnboardingPasskeyScreen.tsx (1)
179-211
: Extract onboarding logic to a separate hook.The
onboardWithPasskey
function combines multiple responsibilities. Consider extracting it to a custom hook for better reusability and testing.Create a new hook:
function usePasskeyOnboarding() { // ... extract state and handlers const onboardWithPasskey = useCallback(async (passkeyName: string) => { // ... existing logic }, [/* dependencies */]); return { onboardWithPasskey, // ... other handlers }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lock
is excluded by!**/*.lock
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (39)
app.json
(1 hunks)components/Avatar.tsx
(1 hunks)components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx
(1 hunks)components/Onboarding/init-xmtp-client.ts
(2 hunks)config.ts
(3 hunks)features/conversation/conversation.nav.tsx
(1 hunks)features/embedded-wallets/embedded-wallet.interface.ts
(1 hunks)features/embedded-wallets/ephemeral-embedded-wallet.ts
(1 hunks)features/embedded-wallets/turnkey-embedded-wallet.ts
(1 hunks)features/onboarding/passkey/passkeyAuthStore.tsx
(1 hunks)i18n/translations/en.ts
(2 hunks)ios/Converse/Converse.entitlements
(1 hunks)navigation/OnboardingNavigator.tsx
(3 hunks)package.json
(2 hunks)screens/Main.tsx
(1 hunks)screens/Navigation/Navigation.tsx
(7 hunks)screens/NewAccount/NewAccountConnectWalletScreen.tsx
(1 hunks)screens/NewAccount/NewAccountEphemeralScreen.tsx
(2 hunks)screens/NewAccount/NewAccountPasskeyScreen.tsx
(1 hunks)screens/NewAccount/NewAccountPrivateKeyScreen.tsx
(1 hunks)screens/NewAccount/NewAccountPrivyScreen.tsx
(1 hunks)screens/NewAccount/NewAccountScreen.tsx
(2 hunks)screens/NewAccount/NewAccountUserProfileScreen.tsx
(1 hunks)screens/Onboarding/OnboardingEphemeralScreen.tsx
(1 hunks)screens/Onboarding/OnboardingGetStartedScreen.tsx
(2 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx
(1 hunks)utils/api.ts
(3 hunks)utils/navigation.ts
(1 hunks)utils/passkeys/add-wallet-to-passkey.ts
(1 hunks)utils/passkeys/create-passkey.ts
(1 hunks)utils/passkeys/create-suborganization.ts
(1 hunks)utils/passkeys/create-turnkey-account.ts
(1 hunks)utils/passkeys/load-client-from-passkey.ts
(1 hunks)utils/passkeys/passkeys.constants.ts
(1 hunks)utils/passkeys/passkeys.interfaces.ts
(1 hunks)utils/passkeys/persist-passkeys.ts
(1 hunks)utils/passkeys/turnkey-clients.ts
(1 hunks)utils/xmtpRN/signIn.ts
(2 hunks)utils/xmtpRN/signer.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- features/conversation/conversation.nav.tsx
- ios/Converse/Converse.entitlements
- screens/Main.tsx
- utils/passkeys/passkeys.constants.ts
- screens/Onboarding/OnboardingGetStartedScreen.tsx
- screens/NewAccount/NewAccountScreen.tsx
- utils/passkeys/turnkey-clients.ts
- utils/navigation.ts
- screens/Onboarding/OnboardingEphemeralScreen.tsx
- utils/passkeys/persist-passkeys.ts
- components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx
- package.json
- utils/passkeys/create-turnkey-account.ts
- features/embedded-wallets/turnkey-embedded-wallet.ts
- features/embedded-wallets/ephemeral-embedded-wallet.ts
- utils/passkeys/passkeys.interfaces.ts
- screens/Navigation/Navigation.tsx
- app.json
- navigation/OnboardingNavigator.tsx
- screens/NewAccount/NewAccountPasskeyScreen.tsx
- utils/passkeys/create-passkey.ts
🔇 Additional comments (12)
screens/NewAccount/NewAccountConnectWalletScreen.tsx (1)
23-23
: LGTM! Navigation behavior improvementUsing
popTo
instead ofnavigate
is a better choice here as it prevents navigation stack buildup during the onboarding flow.screens/NewAccount/NewAccountPrivyScreen.tsx (1)
36-36
: LGTM! Consistent navigation patternThe change to
popTo
aligns with the navigation pattern across screens and properly manages the navigation stack. Error handling is also properly implemented with error tracking.screens/NewAccount/NewAccountPrivateKeyScreen.tsx (1)
34-34
: LGTM! Consistent navigation patternThe change to
popTo
aligns with the navigation pattern across screens.screens/NewAccount/NewAccountUserProfileScreen.tsx (1)
45-45
: LGTM! Clean implementationThe changes maintain consistency with the navigation pattern across screens. The implementation includes proper input validation, error handling, and dependency management.
Also applies to: 50-50
config.ts (2)
79-79
: Ensure consistent domain usage across the application.The domain values are used in multiple places:
- Universal links configuration
- Web credentials
- Passkey authentication
Verify that these new domain configurations are consistently used across the application.
Also applies to: 99-99, 134-134
✅ Verification successful
Domain configuration is properly implemented and consistent
The domain values are consistently configured across all required areas:
- Universal links configuration matches environment-specific domains
- Web credentials use correct domain values per environment
- Deep linking and intent filters are properly configured
- Both primary (.converse.xyz) and alternative (.getconverse.app) domains are consistently handled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for domain usage across the codebase for domain in "dev.converse.xyz" "preview.converse.xyz" "converse.xyz"; do echo "Checking usage of $domain..." rg -l "$domain" doneLength of output: 1050
Script:
#!/bin/bash # Check universal links and authentication related configurations echo "Checking universal links and authentication configurations..." rg -A 3 "associatedDomains|universalLinks|webCredentials|passkey" config.ts app.config.ts echo -e "\nChecking domain usage in config files..." cat config.ts echo -e "\nChecking app config..." cat app.config.tsLength of output: 12412
79-79
: Verify domain configuration for passkey authentication.Since these domains are used for passkey authentication, ensure:
- DNS records are properly configured
- SSL certificates are valid
- Associated entitlements are correctly set up in the iOS configuration
Also applies to: 99-99, 134-134
utils/passkeys/add-wallet-to-passkey.ts (2)
1-11
: LGTM! Clean imports and well-structured type definition.The imports are properly organized, and the
AddWalletToPasskeyParams
type clearly defines the expected parameters.
1-50
: Verify security implications of wallet creationPlease ensure:
- The
RPID
constant is properly configured for the production environment- The wallet creation parameters align with security best practices
- Error messages don't leak sensitive information
✅ Verification successful
Security implementation verified and follows best practices
The implementation is secure with:
- Environment-aware RPID configuration
- Industry-standard wallet creation parameters
- Proper error handling without sensitive data exposure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential security configurations echo "Checking RPID configuration..." rg -A 1 "RPID.*=.*" --glob '**/*.{ts,js}' echo "Checking for potential sensitive data exposure in error messages..." rg "throw.*Error.*" --glob '**/*.{ts,js}' echo "Checking wallet creation parameters across the codebase..." rg "CURVE_SECP256K1|PATH_FORMAT_BIP32|ADDRESS_FORMAT_ETHEREUM" --glob '**/*.{ts,js}'Length of output: 7367
Script:
#!/bin/bash # Check config.appDomain configuration echo "Checking config.appDomain source..." rg -p "config.*appDomain" --glob '**/*.{ts,js}' echo "Checking for any config files..." fd -e ts -e js -g "*config*"Length of output: 426
Script:
#!/bin/bash echo "Checking config.ts content..." cat config.ts echo -e "\nChecking for appDomain references..." rg "appDomain" --glob '**/*.{ts,js}'Length of output: 5737
components/Avatar.tsx (1)
50-50
: Verify the impact of style changes.The removal of
styles.imageContainer
from the View component might affect the layout. Please ensure that all essential styles are properly passed via thestyle
prop.✅ Verification successful
Style changes are safe to proceed
The removal of
styles.imageContainer
doesn't impact the component's functionality as all essential image styling is handled internally, while thestyle
prop is correctly used for external positioning. The component is well-tested and widely used across the codebase with no dependencies on the removed style.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of Avatar component to verify style prop usage rg -A 5 '<Avatar[^>]*style='Length of output: 30
Script:
#!/bin/bash # Search for any Avatar component usage rg '<Avatar' -A 5 # Examine the Avatar component implementation cat components/Avatar.tsxLength of output: 19270
i18n/translations/en.ts (1)
11-11
: LGTM!The new translation key follows consistent naming patterns and provides clear messaging.
screens/NewAccount/NewAccountEphemeralScreen.tsx (2)
Line range hint
28-46
: Well-implemented wallet generation and error handlingThe implementation demonstrates good practices:
- Secure random key generation
- Proper error handling with Sentry tracking
- Clear loading state management
- Clean React hooks usage
40-40
: Verify navigation behavior changeThe navigation method has been changed from
navigate
topopTo
. This changes how the screen stack is managed and affects the back button behavior.Please confirm:
- This change is intentional and part of a broader navigation pattern update
- Users can still navigate back if needed
- No screens are accidentally removed from the stack
✅ Verification successful
Navigation pattern change is consistent with codebase
The change from
navigate
topopTo
is part of a consistent pattern where account creation flows usepopTo("Chats")
to clean up the navigation stack after completing multi-step processes. This is the correct approach as it prevents stack buildup from intermediate screens.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent navigation pattern across related files echo "Checking navigation patterns in related files..." rg --type typescript -B 2 -A 2 'router\.(navigate|popTo)\("Chats"\)' ./screens/NewAccount/Length of output: 234
Script:
#!/bin/bash # Search for navigation patterns to "Chats" screen across the codebase echo "Searching for navigation patterns..." rg -t tsx -t ts -B 2 -A 2 'router\.(navigate|popTo)\("Chats"\)' # Also check for any other references to the Chats screen in navigation echo -e "\nChecking other navigation references to Chats screen..." rg -t tsx -t ts 'Chats"?\s*[,)]' --context 2Length of output: 378
Script:
#!/bin/bash # Search for navigation patterns using find and grep echo "Searching for navigation patterns in TypeScript files..." find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l -A 2 -B 2 'router\.\(navigate\|popTo\)("Chats")' {} \; # Look for screen definitions or routes related to Chats echo -e "\nChecking screen definitions and routes..." find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "screen.*[\"']Chats[\"']" {} \;Length of output: 781
Script:
#!/bin/bash # Check the actual navigation methods used in the identified files echo "Checking navigation methods in the identified files..." for file in ./components/AccountSettingsButton.tsx ./screens/Accounts/Accounts.tsx ./screens/NewAccount/*.tsx; do echo -e "\n=== $file ===" grep -A 1 -B 1 'router\.' "$file" || true doneLength of output: 5081
export const createXmtpClientFromViemAccount = async ( | ||
account: LocalAccount, | ||
onInstallationRevoked: () => Promise<void>, | ||
preAuthenticateToInboxCallback?: () => Promise<void> | ||
) => { | ||
const tempDirectory = await createTemporaryDirectory(); | ||
const dbEncryptionKey = await getDbEncryptionKey(); | ||
|
||
const options = { | ||
env, | ||
enableV3: true, | ||
dbDirectory: tempDirectory, | ||
dbEncryptionKey, | ||
}; | ||
const inboxId = await getInboxId(await account.address); | ||
|
||
await copyDatabasesToTemporaryDirectory(tempDirectory, inboxId); | ||
|
||
logger.debug("Instantiating client from signer"); | ||
|
||
const client = await Client.create(viemAccountToXmtpSigner(account), { | ||
...options, | ||
preAuthenticateToInboxCallback, | ||
}); | ||
|
||
if (client.inboxId !== inboxId) { | ||
throw new Error("Inbox ids don't match"); | ||
} | ||
|
||
// In case we're logging with an existing libxmtp database, make sure | ||
// the installation has not already been revoked | ||
const installationValid = await isClientInstallationValid(client); | ||
if (!installationValid) { | ||
await client.dropLocalDatabaseConnection(); | ||
await deleteLibXmtpDatabaseForInboxId(inboxId); | ||
onInstallationRevoked(); | ||
return; | ||
} | ||
|
||
logger.debug("Instantiated client from signer"); | ||
// This Client is only be used to extract the key, we can disconnect | ||
// it to prevent locks happening during Onboarding | ||
await client.dropLocalDatabaseConnection(); | ||
await moveTemporaryDatabasesToDatabaseDirectory( | ||
tempDirectory, | ||
client.inboxId | ||
); | ||
logger.debug("Dropped client databases"); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Reduce code duplication with createXmtpClientFromSigner
The new function duplicates most of the logic from createXmtpClientFromSigner
. Consider extracting the common functionality into a shared helper function.
Apply this diff to reduce duplication:
+async function createXmtpClientBase<T>(
+ signerOrAccount: T,
+ createSigner: (input: T) => XmtpSigner,
+ onInstallationRevoked: () => Promise<void>,
+ preAuthenticateToInboxCallback?: () => Promise<void>
+) {
+ const tempDirectory = await createTemporaryDirectory();
+ const dbEncryptionKey = await getDbEncryptionKey();
+
+ const options = {
+ env,
+ enableV3: true,
+ dbDirectory: tempDirectory,
+ dbEncryptionKey,
+ };
+
+ const address = typeof signerOrAccount === 'object' && 'address' in signerOrAccount
+ ? signerOrAccount.address
+ : await (signerOrAccount as Signer).getAddress();
+
+ const inboxId = await getInboxId(address);
+
+ await copyDatabasesToTemporaryDirectory(tempDirectory, inboxId);
+
+ logger.debug("Instantiating client from signer");
+
+ const client = await Client.create(createSigner(signerOrAccount), {
+ ...options,
+ preAuthenticateToInboxCallback,
+ });
+
+ if (client.inboxId !== inboxId) {
+ throw new Error("Inbox ids don't match");
+ }
+
+ const installationValid = await isClientInstallationValid(client);
+ if (!installationValid) {
+ await client.dropLocalDatabaseConnection();
+ await deleteLibXmtpDatabaseForInboxId(inboxId);
+ onInstallationRevoked();
+ return;
+ }
+
+ logger.debug("Instantiated client from signer");
+ await client.dropLocalDatabaseConnection();
+ await moveTemporaryDatabasesToDatabaseDirectory(tempDirectory, client.inboxId);
+ logger.debug("Dropped client databases");
+}
export const createXmtpClientFromSigner = async (
signer: Signer,
onInstallationRevoked: () => Promise<void>,
preAuthenticateToInboxCallback?: () => Promise<void>
-) => {
- const tempDirectory = await createTemporaryDirectory();
- // ... rest of the implementation
+) => createXmtpClientBase(
+ signer,
+ (s) => ethersSignerToXmtpSigner(s),
+ onInstallationRevoked,
+ preAuthenticateToInboxCallback
+);
export const createXmtpClientFromViemAccount = async (
account: LocalAccount,
onInstallationRevoked: () => Promise<void>,
preAuthenticateToInboxCallback?: () => Promise<void>
-) => {
- const tempDirectory = await createTemporaryDirectory();
- // ... rest of the implementation
+) => createXmtpClientBase(
+ account,
+ (a) => viemAccountToXmtpSigner(a),
+ onInstallationRevoked,
+ preAuthenticateToInboxCallback
+);
Committable suggestion skipped: line range outside the PR's diff.
if (!account || !address) { | ||
throw new Error("No signer or address"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix incorrect error message
The error message refers to "signer" but the function checks for "account".
Apply this diff to fix the error message:
if (!account || !address) {
- throw new Error("No signer or address");
+ throw new Error("No account or address");
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
if (!account || !address) { | |
throw new Error("No signer or address"); | |
} | |
if (!account || !address) { | |
throw new Error("No account or address"); | |
} |
export type IEmbeddedWallet = { | ||
account: Account | null; | ||
|
||
createAccount: () => Promise<Account>; | ||
|
||
loadAccount: () => Promise<Account>; | ||
|
||
xmtpClient: Client | null; | ||
|
||
instantiateXmtpClient: () => Promise<Client>; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add documentation and error handling patterns
The interface would benefit from the following improvements:
- Add JSDoc comments describing the purpose and requirements of each method
- Define error types/patterns for async operations
- Add a cleanup method for proper XMTP client resource management
Here's a suggested implementation:
+/** Represents an embedded wallet with account and XMTP messaging capabilities */
export type IEmbeddedWallet = {
account: Account | null;
+ /**
+ * Creates a new wallet account
+ * @throws {WalletCreationError} If account creation fails
+ * @returns Promise resolving to the created account
+ */
createAccount: () => Promise<Account>;
+ /**
+ * Loads an existing wallet account
+ * @throws {WalletLoadError} If account loading fails
+ * @returns Promise resolving to the loaded account
+ */
loadAccount: () => Promise<Account>;
xmtpClient: Client | null;
+ /**
+ * Instantiates an XMTP client for messaging
+ * @throws {XmtpError} If client instantiation fails
+ * @returns Promise resolving to the XMTP client
+ */
instantiateXmtpClient: () => Promise<Client>;
+
+ /**
+ * Cleanup resources and disconnect clients
+ * @returns Promise resolving when cleanup is complete
+ */
+ cleanup: () => Promise<void>;
};
Additionally, consider creating custom error types for better error handling:
export class WalletCreationError extends Error {
constructor(message: string, public readonly cause?: unknown) {
super(message);
this.name = 'WalletCreationError';
}
}
// Similar error classes for WalletLoadError and XmtpError
import { Client } from "@xmtp/react-native-sdk"; | ||
import { LocalAccount } from "viem/accounts"; | ||
|
||
type Account = LocalAccount<any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Use LocalAccount<"privateKey">
instead of LocalAccount<any>
The LocalAccount<"privateKey">
type is used in the concrete implementation (EphemeralEmbeddedWallet
), which aligns with the account creation flow using privateKeyToAccount
. Using any
reduces type safety and doesn't accurately represent the expected type parameter.
🔗 Analysis chain
Consider specifying the type parameter for LocalAccount
The any
type parameter in LocalAccount<any>
reduces type safety. Consider specifying the exact type parameter expected by the LocalAccount generic type to ensure better type checking and documentation.
Let's check the viem documentation and usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for LocalAccount usage patterns in the codebase
rg "LocalAccount<" --type ts
Length of output: 279
Script:
#!/bin/bash
# Check the implementation details of ephemeral wallet
cat features/embedded-wallets/ephemeral-embedded-wallet.ts
Length of output: 1636
Script:
#!/bin/bash
# Search for account-related imports from viem
rg "from 'viem/accounts'" --type ts -A 2
Length of output: 48
Script:
#!/bin/bash
# Search for viem imports with double quotes and broader pattern
rg 'from "viem/' --type ts -A 2
# Also search for privateKeyToAccount usage as it's relevant
rg "privateKeyToAccount" --type ts
Length of output: 1995
const address = res.activity.result.createWalletResult?.addresses[0]; | ||
if (!address) { | ||
throw new Error("No address found"); | ||
} | ||
const account = await createTurnkeyAccount(address, subOrgId); | ||
return { address, account }; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling and add retry mechanism
The current error handling could expose sensitive information and lacks retry capability for transient failures.
+const MAX_RETRIES = 3;
+const RETRY_DELAY = 1000; // ms
+
+const sleep = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));
+
+async function withRetry<T>(
+ operation: () => Promise<T>,
+ retries: number = MAX_RETRIES
+): Promise<T> {
+ try {
+ return await operation();
+ } catch (error) {
+ if (retries > 0) {
+ await sleep(RETRY_DELAY);
+ return withRetry(operation, retries - 1);
+ }
+ throw new Error("Operation failed after multiple retries");
+ }
+}
+
- const address = res.activity.result.createWalletResult?.addresses[0];
+ const address = res.activity.result.createWalletResult?.addresses?.[0];
if (!address) {
- throw new Error("No address found");
+ throw new Error("Wallet creation failed: address not generated");
}
- const account = await createTurnkeyAccount(address, subOrgId);
+ const account = await withRetry(() => createTurnkeyAccount(address, subOrgId));
return { address, account };
Committable suggestion skipped: line range outside the PR's diff.
return createStore<IPasskeyAuthStoreState>()((set) => ({ | ||
...DEFAULT_PROPS, | ||
...initProps, | ||
setLoading: (loading) => | ||
loading ? set({ loading, error: undefined }) : set({ loading: false }), | ||
setError: (error) => set({ error, statusString: undefined }), | ||
setStatusString: (statusString) => set({ statusString }), | ||
setAccount: (account) => set({ account }), | ||
setTurnkeyInfo: (turnkeyInfo) => set({ turnkeyInfo }), | ||
setPreviousPasskeyName: (previousPasskeyName) => | ||
set({ previousPasskeyName }), | ||
reset: () => set(DEFAULT_PROPS), | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add state update validation.
The state setters don't validate input, which could lead to inconsistent state.
Add validation to prevent invalid state updates:
setTurnkeyInfo: (turnkeyInfo) =>
- set({ turnkeyInfo }),
+ set((state) => {
+ if (turnkeyInfo && (!turnkeyInfo.subOrganizationId || !turnkeyInfo.walletId)) {
+ console.warn('Invalid turnkey info provided');
+ return state;
+ }
+ return { turnkeyInfo };
+ }),
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
return createStore<IPasskeyAuthStoreState>()((set) => ({ | |
...DEFAULT_PROPS, | |
...initProps, | |
setLoading: (loading) => | |
loading ? set({ loading, error: undefined }) : set({ loading: false }), | |
setError: (error) => set({ error, statusString: undefined }), | |
setStatusString: (statusString) => set({ statusString }), | |
setAccount: (account) => set({ account }), | |
setTurnkeyInfo: (turnkeyInfo) => set({ turnkeyInfo }), | |
setPreviousPasskeyName: (previousPasskeyName) => | |
set({ previousPasskeyName }), | |
reset: () => set(DEFAULT_PROPS), | |
})); | |
return createStore<IPasskeyAuthStoreState>()((set) => ({ | |
...DEFAULT_PROPS, | |
...initProps, | |
setLoading: (loading) => | |
loading ? set({ loading, error: undefined }) : set({ loading: false }), | |
setError: (error) => set({ error, statusString: undefined }), | |
setStatusString: (statusString) => set({ statusString }), | |
setAccount: (account) => set({ account }), | |
setTurnkeyInfo: (turnkeyInfo) => | |
set((state) => { | |
if (turnkeyInfo && (!turnkeyInfo.subOrganizationId || !turnkeyInfo.walletId)) { | |
console.warn('Invalid turnkey info provided'); | |
return state; | |
} | |
return { turnkeyInfo }; | |
}), | |
setPreviousPasskeyName: (previousPasskeyName) => | |
set({ previousPasskeyName }), | |
reset: () => set(DEFAULT_PROPS), | |
})); |
export function NewAccountEphemeralScreen( | ||
props: NativeStackScreenProps<NavigationParamList, "NewAccountEphemera"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix type definition to match component name
The route parameter in NativeStackScreenProps
still uses "NewAccountEphemera"
while the component has been renamed to NewAccountEphemeralScreen
. This mismatch could cause type errors.
Update the type definition to match the new name:
export function NewAccountEphemeralScreen(
- props: NativeStackScreenProps<NavigationParamList, "NewAccountEphemera">
+ props: NativeStackScreenProps<NavigationParamList, "NewAccountEphemeral">
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export function NewAccountEphemeralScreen( | |
props: NativeStackScreenProps<NavigationParamList, "NewAccountEphemera"> | |
export function NewAccountEphemeralScreen( | |
props: NativeStackScreenProps<NavigationParamList, "NewAccountEphemeral"> |
c17fe74
to
de66f20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
♻️ Duplicate comments (9)
utils/passkeys/create-passkey.ts (3)
53-56
: 🛠️ Refactor suggestionEnhance error handling with specific error messages
The current error handling only logs and rethrows errors without providing user-friendly feedback.
} catch (e) { console.error("error during passkey creation", e); + setStatusString(`Failed to create passkey: ${e instanceof Error ? e.message : 'Unknown error'}`); throw e; }
59-62
:⚠️ Potential issueAdd early return for unsupported devices
The function continues execution even when passkeys are not supported.
if (!isSupported()) { alert("Passkeys are not supported on this device"); + return Promise.reject(new Error("Passkeys not supported")); }
67-88
:⚠️ Potential issueAdd error handling for createPasskey operation
The
createPasskey
call lacks error handling.- const authenticatorParams = await createPasskey({ + let authenticatorParams; + try { + authenticatorParams = await createPasskey({ authenticatorName: "End-User Passkey", rp: { id: RPID, name: "Passkey App", }, user: { id: userId, name: passkeyName + " " + humanReadableDate, displayName: passkeyName + " " + humanReadableDate, }, authenticatorSelection: { residentKey: "required", requireResidentKey: true, userVerification: "preferred", }, - }); + }); + } catch (error) { + console.error("Failed to create passkey:", error); + throw new Error( + `Passkey creation failed: ${error instanceof Error ? error.message : 'Unknown error'}` + ); + }utils/passkeys/turnkey-clients.ts (2)
7-9
:⚠️ Potential issueAdd error handling for missing clients
The getter should handle the case when a client doesn't exist for the given
subOrgId
.export const getTurnkeyClient = (subOrgId: string) => { + if (!turnkeyClientMap[subOrgId]) { + throw new Error(`No TurnkeyClient found for subOrgId: ${subOrgId}`); + } return turnkeyClientMap[subOrgId]; };
11-16
:⚠️ Potential issueAdd input validation for setTurnkeyClient
The setter should validate its inputs to prevent potential issues.
export const setTurnkeyClient = ( subOrgId: string, turnkeyClient: TurnkeyClient ) => { + if (!subOrgId?.trim()) { + throw new Error('subOrgId is required'); + } + if (!turnkeyClient) { + throw new Error('turnkeyClient is required'); + } turnkeyClientMap[subOrgId] = turnkeyClient; };utils/passkeys/load-client-from-passkey.ts (2)
26-28
: 🛠️ Refactor suggestionMove API URL to environment configuration
Hardcoding the API URL is not a good practice. Move it to environment configuration.
const client = new TurnkeyClient( - { baseUrl: "https://api.turnkey.com" }, + { baseUrl: config.turnkeyApiUrl }, stamper );
67-70
:⚠️ Potential issueEnhance error handling to prevent information leakage
The current error handling logs the entire error object, which might expose sensitive information.
} catch (e) { - console.error("error during passkey load", e); - throw e; + console.error("Error during passkey load:", e instanceof Error ? e.message : "Unknown error"); + throw new Error("Failed to load passkey"); }screens/NewAccount/NewAccountPasskeyScreen.tsx (2)
167-174
:⚠️ Potential issueImprove error handling in createXmtpClientFromAccount
The error handling concatenates different error types unsafely and uses console.log instead of console.error.
- console.log("error creating Xmtp client", err); + console.error("Error creating Xmtp client:", err); setStatusString(""); - handleError( - "Error creating Xmtp client : " + - (err instanceof Error ? err.message : "") + - (typeof err === "string" ? err : "") - ); + handleError( + `Error creating Xmtp client: ${ + err instanceof Error ? err.message : + typeof err === "string" ? err : + "Unknown error" + }` + );
284-289
:⚠️ Potential issueAdd input validation for passkey name
The TextField for passkey name lacks validation and error handling.
<TextField placeholder="Enter your passkey" + error={inputTextRef.current?.length < 3 ? "Passkey name must be at least 3 characters" : undefined} onChangeText={(text) => { inputTextRef.current = text; }} + onBlur={() => { + if (!inputTextRef.current) { + setError("Passkey name is required"); + } + }} />
🧹 Nitpick comments (6)
utils/passkeys/create-passkey.ts (2)
25-51
: Improve status messages for better user experienceThe status messages could be more user-friendly and informative.
- setStatusString("Creating passkey..."); + setStatusString("Setting up your secure passkey..."); - setStatusString("Passkey creation successful. Creating sub-org..."); + setStatusString("Passkey created successfully! Setting up your account..."); - setStatusString("Sub-org created! Creating turnkey account..."); + setStatusString("Account setup in progress..."); - setStatusString(`Turnkey account created! Address is ${address}`); + setStatusString(`Setup complete! Your secure wallet address is ${address}`);
64-66
: Improve user ID generation to prevent collisionsUsing timestamp alone for ID generation could lead to collisions in high-concurrency scenarios.
- const userId = Buffer.from(String(Date.now())).toString("base64"); + const userId = Buffer.from( + `${Date.now()}-${Math.random().toString(36).substr(2, 9)}` + ).toString("base64");utils/passkeys/turnkey-clients.ts (1)
20-22
: Simplify boolean checkRemove redundant double negation.
- if (!!client) { + if (client) { return client; }🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
utils/passkeys/load-client-from-passkey.ts (2)
43-50
: Improve embedded wallet detectionThe current method of finding the embedded wallet could be improved.
- // TODO: Is there a better way to find the embedded wallet? - const embeddedWallet = getWallets.wallets.find( - (wallet) => wallet.walletName === "Embedded Wallet" - ); + const EMBEDDED_WALLET_NAME = "Embedded Wallet"; + const embeddedWallet = getWallets.wallets.find( + (wallet) => wallet.walletName === EMBEDDED_WALLET_NAME + ); if (!embeddedWallet) { - throw new Error("No embedded wallet found"); + throw new Error(`No wallet found with name: ${EMBEDDED_WALLET_NAME}`); }
9-18
: Remove unused parameterThe
setPreviousPasskeyName
parameter is defined but never used.type LoadAccountFromPasskeyParams = { setStatusString: (statusString: string) => void; - setPreviousPasskeyName: (passkeyName: string) => void; setTurnkeyInfo: (turnkeyInfo: TurnkeyStoreInfo) => void; }; export const loadAccountFromPasskey = async ({ setStatusString, setTurnkeyInfo, }: LoadAccountFromPasskeyParams) => {
screens/NewAccount/NewAccountPasskeyScreen.tsx (1)
1-20
: Consider organizing imports by categoryConsider grouping imports into categories for better maintainability:
- React/Framework imports
- Components
- Features/Utils
- Design system
import React, { memo, useCallback, useRef } from "react"; + import { NewAccountScreenComp } from "@components/NewAccount/NewAccountScreenComp"; import { NewAccountPictoTitleSubtitle } from "@components/NewAccount/NewAccountTitleSubtitlePicto"; + import { PasskeyAuthStoreProvider, usePasskeyAuthStoreContext, } from "@features/onboarding/passkey/passkeyAuthStore"; import { translate } from "@i18n"; import { useRouter } from "../../navigation/useNavigation"; import { isMissingConverseProfile } from "../Onboarding/Onboarding.utils"; import { addWalletToPasskey } from "@/utils/passkeys/add-wallet-to-passkey"; import { initXmtpClientFromViemAccount } from "@/components/Onboarding/init-xmtp-client"; + import { Button } from "@/design-system/Button/Button"; import { TextField } from "@/design-system/TextField/TextField"; import { Text } from "@/design-system/Text";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
App.tsx
(0 hunks)config.ts
(4 hunks)screens/Main.tsx
(3 hunks)screens/NewAccount/NewAccountPasskeyScreen.tsx
(1 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx
(1 hunks)utils/passkeys/create-passkey.ts
(1 hunks)utils/passkeys/create-turnkey-account.ts
(1 hunks)utils/passkeys/load-client-from-passkey.ts
(1 hunks)utils/passkeys/passkeys.constants.ts
(1 hunks)utils/passkeys/passkeys.interfaces.ts
(1 hunks)utils/passkeys/turnkey-clients.ts
(1 hunks)
💤 Files with no reviewable changes (1)
- App.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- utils/passkeys/passkeys.constants.ts
- screens/Main.tsx
- config.ts
- utils/passkeys/passkeys.interfaces.ts
- screens/Onboarding/OnboardingPasskeyScreen.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
utils/passkeys/turnkey-clients.ts
[error] 20-20: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (1)
utils/passkeys/create-passkey.ts (1)
11-16
: Well-structured type definition!The
PasskeyCreateParams
type clearly defines the required parameters with appropriate callback functions for status updates and state management.
const turnkeyAccount = await createTurnkeyAccount( | ||
address, | ||
subOrganizationId! | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove non-null assertion operator
The non-null assertion operator (!
) on subOrganizationId
could lead to runtime errors if the value is undefined. Consider adding a validation check instead.
- subOrganizationId!
+ subOrganizationId ?? throw new Error("SubOrganizationId is required")
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const turnkeyAccount = await createTurnkeyAccount( | |
address, | |
subOrganizationId! | |
); | |
const turnkeyAccount = await createTurnkeyAccount( | |
address, | |
subOrganizationId ?? throw new Error("SubOrganizationId is required") | |
); |
rpId: RPID, | ||
}); | ||
|
||
client = new TurnkeyClient({ baseUrl: "https://api.turnkey.com" }, stamper); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Move API URL to environment configuration
Hardcoding the API URL is not a good practice. Move it to environment configuration.
- client = new TurnkeyClient({ baseUrl: "https://api.turnkey.com" }, stamper);
+ client = new TurnkeyClient({ baseUrl: config.turnkeyApiUrl }, stamper);
Committable suggestion skipped: line range outside the PR's diff.
export const createTurnkeyAccountFromWalletId = async ( | ||
walletId: string, | ||
subOrganizationId: string | ||
) => { | ||
try { | ||
const client = getOrCreateTurnkeyClient(subOrganizationId); | ||
|
||
const account = await client.getWalletAccounts({ | ||
organizationId: subOrganizationId, | ||
walletId, | ||
}); | ||
const address = account.accounts[0].address; | ||
|
||
return createTurnkeyAccount(address, subOrganizationId); | ||
} catch (e) { | ||
console.error("error during turnkey account creation", e); | ||
throw e; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add JSDoc documentation and improve error handling
The function lacks documentation and has generic error handling.
+/**
+ * Creates a Turnkey account from a wallet ID.
+ * @param walletId - The ID of the wallet to create the account from.
+ * @param subOrganizationId - The sub-organization ID to create the account for.
+ * @returns The created Turnkey account.
+ * @throws {Error} If no accounts are found for the wallet.
+ */
export const createTurnkeyAccountFromWalletId = async (
walletId: string,
subOrganizationId: string
) => {
try {
const client = getOrCreateTurnkeyClient(subOrganizationId);
const account = await client.getWalletAccounts({
organizationId: subOrganizationId,
walletId,
});
+ if (!account.accounts?.length) {
+ throw new Error('No accounts found for the wallet');
+ }
const address = account.accounts[0].address;
return createTurnkeyAccount(address, subOrganizationId);
} catch (e) {
- console.error("error during turnkey account creation", e);
- throw e;
+ console.error("Error during turnkey account creation:", e instanceof Error ? e.message : "Unknown error");
+ throw new Error("Failed to create Turnkey account");
}
};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export const createTurnkeyAccountFromWalletId = async ( | |
walletId: string, | |
subOrganizationId: string | |
) => { | |
try { | |
const client = getOrCreateTurnkeyClient(subOrganizationId); | |
const account = await client.getWalletAccounts({ | |
organizationId: subOrganizationId, | |
walletId, | |
}); | |
const address = account.accounts[0].address; | |
return createTurnkeyAccount(address, subOrganizationId); | |
} catch (e) { | |
console.error("error during turnkey account creation", e); | |
throw e; | |
} | |
}; | |
/** | |
* Creates a Turnkey account from a wallet ID. | |
* @param walletId - The ID of the wallet to create the account from. | |
* @param subOrganizationId - The sub-organization ID to create the account for. | |
* @returns The created Turnkey account. | |
* @throws {Error} If no accounts are found for the wallet. | |
*/ | |
export const createTurnkeyAccountFromWalletId = async ( | |
walletId: string, | |
subOrganizationId: string | |
) => { | |
try { | |
const client = getOrCreateTurnkeyClient(subOrganizationId); | |
const account = await client.getWalletAccounts({ | |
organizationId: subOrganizationId, | |
walletId, | |
}); | |
if (!account.accounts?.length) { | |
throw new Error('No accounts found for the wallet'); | |
} | |
const address = account.accounts[0].address; | |
return createTurnkeyAccount(address, subOrganizationId); | |
} catch (e) { | |
console.error("Error during turnkey account creation:", e instanceof Error ? e.message : "Unknown error"); | |
throw new Error("Failed to create Turnkey account"); | |
} | |
}; |
{turnkeyInfo && ( | ||
<Text preset="bodyBold"> | ||
Turnkey info: | ||
<Text preset="body">{JSON.stringify(turnkeyInfo)}</Text> | ||
</Text> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security: Remove sensitive information from UI
Displaying turnkey information in the UI could pose a security risk if screenshots are taken or if the screen is visible to others.
- {turnkeyInfo && (
- <Text preset="bodyBold">
- Turnkey info:
- <Text preset="body">{JSON.stringify(turnkeyInfo)}</Text>
- </Text>
- )}
+ {turnkeyInfo && (
+ <Text preset="bodyBold">
+ Passkey configured successfully
+ </Text>
+ )}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
{turnkeyInfo && ( | |
<Text preset="bodyBold"> | |
Turnkey info: | |
<Text preset="body">{JSON.stringify(turnkeyInfo)}</Text> | |
</Text> | |
)} | |
{turnkeyInfo && ( | |
<Text preset="bodyBold"> | |
Passkey configured successfully | |
</Text> | |
)} |
<Button | ||
text={"Onboard with passkey"} | ||
onPress={onboardWithPasskey} | ||
loading={loading} | ||
/> | ||
<Button | ||
text={"Add new wallet to existing passkey"} | ||
onPress={addWalletToExistingPasskey} | ||
loading={loading} | ||
/> | ||
<Button | ||
text={translate("passkey.createButton")} | ||
onPress={handleCreatePasskey} | ||
loading={loading} | ||
variant="secondary" | ||
/> | ||
<Button | ||
text={"Login with passkey"} | ||
onPress={handleLoginWithPasskey} | ||
loading={loading} | ||
variant="secondary" | ||
/> | ||
<Button | ||
text={"Add new wallet to passkey"} | ||
onPress={handleAddWalletToPasskey} | ||
loading={loading} | ||
variant="secondary" | ||
/> | ||
<Button | ||
text={"Create Xmtp client from account"} | ||
onPress={createXmtpClientFromAccount} | ||
loading={loading} | ||
variant="secondary" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve loading state management for multiple operations
Currently, all buttons share the same loading state, which could lead to unclear UI feedback when multiple operations are in progress. Consider using separate loading states for each operation.
+ const [createLoading, setCreateLoading] = useState(false);
+ const [loginLoading, setLoginLoading] = useState(false);
+ const [addWalletLoading, setAddWalletLoading] = useState(false);
+ const [xmtpLoading, setXmtpLoading] = useState(false);
const handleCreatePasskey = useCallback(async () => {
try {
- setLoading(true);
+ setCreateLoading(true);
// ... existing code ...
} finally {
- setLoading(false);
+ setCreateLoading(false);
}
}, [...]);
// Update other handlers similarly
return (
// ... existing code ...
<Button
text={"Onboard with passkey"}
onPress={onboardWithPasskey}
- loading={loading}
+ loading={createLoading}
/>
// Update other buttons similarly
);
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
♻️ Duplicate comments (3)
utils/passkeys/create-passkey.ts (3)
46-49
:⚠️ Potential issueRemove non-null assertion operator.
The non-null assertion operator (
!
) onsubOrganizationId
could lead to runtime errors if the value is undefined.Apply this diff:
- subOrganizationId! + subOrganizationId ?? throw new Error("SubOrganizationId is required")
52-55
: 🛠️ Refactor suggestionEnhance error handling with specific error cases.
The current error handling only logs and rethrows errors without providing meaningful user feedback.
Consider handling specific error cases:
} catch (e) { console.error("error during passkey creation", e); if (e instanceof WebAuthnError) { setStatusString("Failed to create passkey: User cancelled or device error"); } else if (e instanceof SubOrgError) { setStatusString("Failed to create organization"); } else { setStatusString("An unexpected error occurred"); } throw e; }
58-61
:⚠️ Potential issuePrevent execution when passkeys are unsupported.
The function continues execution even when passkeys are not supported.
Apply this diff:
if (!isSupported()) { alert("Passkeys are not supported on this device"); + return; }
🧹 Nitpick comments (6)
utils/passkeys/create-passkey.ts (1)
65-65
: Improve userId generation to prevent potential conflicts.Using timestamp alone for userId could lead to conflicts if multiple requests occur simultaneously.
Consider using a more robust unique identifier:
- const userId = Buffer.from(String(Date.now())).toString("base64"); + const userId = Buffer.from( + `${Date.now()}-${Math.random().toString(36).substr(2, 9)}` + ).toString("base64");.github/workflows/build-internal.yml (1)
69-69
: Consider grouping related environment variables togetherWhile the addition of
EXPO_PUBLIC_TURNKEY_ORG
is correct, consider grouping it with other authentication-related variables likeEXPO_PUBLIC_PRIVY_APP_ID
for better organization.Also, consider adding a validation check for the secret:
- echo "EXPO_PUBLIC_TURNKEY_ORG=${{ secrets.EXPO_PUBLIC_TURNKEY_ORG }}" >> $env_file + if [ -z "${{ secrets.EXPO_PUBLIC_TURNKEY_ORG }}" ]; then + echo "Error: EXPO_PUBLIC_TURNKEY_ORG secret is not set" + exit 1 + fi + echo "EXPO_PUBLIC_TURNKEY_ORG=${{ secrets.EXPO_PUBLIC_TURNKEY_ORG }}" >> $env_file.github/workflows/build-app-release.yml (1)
51-51
: Consider DRY principle for environment setupThe same environment variable setup is duplicated in both Android and iOS jobs. Consider extracting this into a reusable composite action or workflow template to maintain DRY principles.
Example approach:
# .github/actions/setup-env/action.yml name: 'Setup Environment' description: 'Sets up environment variables from secrets' runs: using: "composite" steps: - run: | # Your environment setup script echo "EXPO_PUBLIC_CONTACT_ADDRESS=${{ secrets.EXPO_PUBLIC_CONTACT_ADDRESS }}" >> .env.production # ... other variables ... echo "EXPO_PUBLIC_TURNKEY_ORG=${{ secrets.EXPO_PUBLIC_TURNKEY_ORG }}" >> .env.production shell: bashThen use it in both jobs:
- uses: ./.github/actions/setup-envAlso applies to: 95-95
screens/Onboarding/OnboardingPasskeyScreen.tsx (3)
36-68
: Consider using operation-specific loading states.Using a shared loading state for all operations could lead to race conditions if multiple operations are triggered simultaneously.
Consider implementing separate loading states for each operation:
-const loading = usePasskeyAuthStoreContext((state) => state.loading); +const createLoading = usePasskeyAuthStoreContext((state) => state.createLoading); +const loginLoading = usePasskeyAuthStoreContext((state) => state.loginLoading); +const addWalletLoading = usePasskeyAuthStoreContext((state) => state.addWalletLoading); +const xmtpLoading = usePasskeyAuthStoreContext((state) => state.xmtpLoading);
70-76
: Enhance error handling with more descriptive messages.While the error handling is consistent, consider providing more user-friendly error messages and specific error codes for better debugging.
Consider enhancing the error handling:
const handleError = useCallback( (error: string) => { + const errorCode = generateErrorCode(); + const userFriendlyMessage = translateError(error); setError(error); - captureErrorWithToast(error); + captureErrorWithToast({ + message: userFriendlyMessage, + errorCode, + technicalDetails: error, + }); }, [setError] );
262-290
: Update button loading states.The buttons share a common loading state, which could lead to a confusing user experience.
<Button text={"Onboard with passkey"} onPress={onboardWithPasskey} - loading={loading} + loading={createLoading || xmtpLoading} /> <Button text={translate("passkey.createButton")} onPress={handleCreatePasskey} - loading={loading} + loading={createLoading} variant="secondary" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/build-app-release.yml
(2 hunks).github/workflows/build-internal.yml
(1 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx
(1 hunks)scripts/build/ios/preview.js
(1 hunks)scripts/build/ios/production.js
(1 hunks)utils/passkeys/create-passkey.ts
(1 hunks)
🔇 Additional comments (9)
utils/passkeys/create-passkey.ts (2)
1-16
: LGTM! Well-structured imports and type definitions.The imports are properly organized, and the
PasskeyCreateParams
type is well-defined with clear parameter types.
67-80
: LGTM! Well-documented configuration.The code includes clear and helpful comments explaining the purpose of each configuration option.
.github/workflows/build-app-release.yml (2)
Line range hint
8-14
: Verify the removal of scheduled buildsThe removal of scheduled builds (Tuesdays and Thursdays) appears intentional based on the comment. Please confirm this aligns with the team's current release strategy.
53-53
:⚠️ Potential issueFix typo in build script path
There's a typo in the script path that will cause the build to fail:
- run: node scripts/Øbuild/eas.js --env production + run: node scripts/build/eas.js --env productionLikely invalid or redundant comment.
scripts/build/ios/production.js (1)
93-97
: LGTM! Verify domain configuration for passkey support.The addition of web credentials for the production domain is correct and aligns with Apple's requirements for passkey implementation.
Please ensure that:
- The domain
converse.xyz
has the appropriate Apple App Site Association (AASA) file configured- The AASA file includes the
webcredentials
component with the correct app ID- The file is accessible at
https://converse.xyz/.well-known/apple-app-site-association
Run this script to verify the AASA configuration:
✅ Verification successful
✅ Domain configuration verified for passkey support
The Apple App Site Association file is properly configured with the required
webcredentials
component and correct app IDs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify AASA file configuration for passkey support # Note: The actual file content should be validated manually as it may contain sensitive information # Check if AASA file is accessible and properly formatted curl -v https://converse.xyz/.well-known/apple-app-site-association -H "Accept: application/json"Length of output: 2937
scripts/build/ios/preview.js (1)
120-124
: LGTM! Verify preview domain configuration and update documentation.The web credentials configuration for the preview environment is correct and maintains proper environment separation.
Please ensure that:
- The domain
preview.converse.xyz
has the appropriate AASA file configured- The AASA file includes the
webcredentials
component with the correct app ID- The file is accessible at
https://preview.converse.xyz/.well-known/apple-app-site-association
Consider adding documentation in the script's header comment section about the passkey-related domain configurations and their requirements for each environment.
Run this script to verify the AASA configuration:
✅ Verification successful
AASA configuration verified successfully for preview.converse.xyz
The Apple App Site Association file is properly configured with the correct webcredentials for both app bundles in the preview environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify AASA file configuration for preview environment # Note: The actual file content should be validated manually as it may contain sensitive information # Check if AASA file is accessible and properly formatted curl -v https://preview.converse.xyz/.well-known/apple-app-site-association -H "Accept: application/json"Length of output: 3084
screens/Onboarding/OnboardingPasskeyScreen.tsx (3)
1-31
: LGTM! Well-structured component setup.The component follows React best practices with proper separation of concerns, memoization, and clear organization of imports.
160-190
: 🛠️ Refactor suggestionImprove error handling in createXmtpClientFromAccount.
The error handling uses string concatenation and console.log, which could be improved.
} catch (err) { - console.log("error creating Xmtp client", err); + const errorMessage = err instanceof Error ? err.message : + typeof err === "string" ? err : "Unknown error"; setStatusString(""); - handleError( - "Error creating Xmtp client : " + - (err instanceof Error ? err.message : "") + - (typeof err === "string" ? err : "") - ); + handleError(`Error creating Xmtp client: ${errorMessage}`); }Likely invalid or redundant comment.
78-103
:⚠️ Potential issueAdd input validation for passkey name.
The passkey creation lacks validation for the input name, which could lead to security issues.
Add validation before creating the passkey:
const handleCreatePasskey = useCallback(async () => { try { setLoading(true); + const passkeyName = inputTextRef.current ?? ""; + if (!passkeyName.trim()) { + throw new Error("Passkey name is required"); + } + if (passkeyName.length < 3) { + throw new Error("Passkey name must be at least 3 characters"); + } + if (!/^[a-zA-Z0-9-_]+$/.test(passkeyName)) { + throw new Error("Passkey name can only contain letters, numbers, hyphens and underscores"); + } const account = await onPasskeyCreate({ - passkeyName: inputTextRef.current ?? "", + passkeyName, setStatusString, setPreviousPasskeyName, setTurnkeyInfo, });Likely invalid or redundant comment.
{turnkeyInfo && ( | ||
<Text preset="bodyBold"> | ||
Turnkey info: | ||
<Text preset="body">{JSON.stringify(turnkeyInfo)}</Text> | ||
</Text> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove sensitive data from UI.
Displaying raw turnkey info in the UI could expose sensitive data.
Consider removing or masking sensitive information:
- {turnkeyInfo && (
- <Text preset="bodyBold">
- Turnkey info:
- <Text preset="body">{JSON.stringify(turnkeyInfo)}</Text>
- </Text>
- )}
+ {turnkeyInfo && (
+ <Text preset="bodyBold">
+ Turnkey status:
+ <Text preset="body">Connected</Text>
+ </Text>
+ )}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
{turnkeyInfo && ( | |
<Text preset="bodyBold"> | |
Turnkey info: | |
<Text preset="body">{JSON.stringify(turnkeyInfo)}</Text> | |
</Text> | |
)} | |
{turnkeyInfo && ( | |
<Text preset="bodyBold"> | |
Turnkey status: | |
<Text preset="body">Connected</Text> | |
</Text> | |
)} |
<TextField | ||
placeholder="Enter your passkey" | ||
onChangeText={(text) => { | ||
inputTextRef.current = text; | ||
}} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add accessibility support to TextField.
The text field lacks accessibility props, which could make it difficult for users with disabilities.
<TextField
placeholder="Enter your passkey"
+ accessibilityLabel="Passkey input field"
+ accessibilityHint="Enter a name for your passkey"
onChangeText={(text) => {
inputTextRef.current = text;
}}
+ returnKeyType="done"
+ autoCapitalize="none"
+ autoCorrect={false}
/>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
<TextField | |
placeholder="Enter your passkey" | |
onChangeText={(text) => { | |
inputTextRef.current = text; | |
}} | |
/> | |
<TextField | |
placeholder="Enter your passkey" | |
accessibilityLabel="Passkey input field" | |
accessibilityHint="Enter a name for your passkey" | |
onChangeText={(text) => { | |
inputTextRef.current = text; | |
}} | |
returnKeyType="done" | |
autoCapitalize="none" | |
autoCorrect={false} | |
/> |
const handleLoginWithPasskey = useCallback(async () => { | ||
try { | ||
setLoading(true); | ||
const { account } = await loadAccountFromPasskey({ | ||
setStatusString, | ||
setPreviousPasskeyName, | ||
setTurnkeyInfo, | ||
}); | ||
if (!account) { | ||
handleError("No account loaded from Passkey"); | ||
throw new Error("No account loaded from Passkey"); | ||
} | ||
setAccount(account); | ||
} catch (e) { | ||
handleError(e instanceof Error ? e.message : "Unknown error"); | ||
} finally { | ||
setLoading(false); | ||
} | ||
}, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove redundant error handling in handleLoginWithPasskey.
The function throws an error after already handling it with handleError
, which is redundant.
if (!account) {
handleError("No account loaded from Passkey");
- throw new Error("No account loaded from Passkey");
return;
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const handleLoginWithPasskey = useCallback(async () => { | |
try { | |
setLoading(true); | |
const { account } = await loadAccountFromPasskey({ | |
setStatusString, | |
setPreviousPasskeyName, | |
setTurnkeyInfo, | |
}); | |
if (!account) { | |
handleError("No account loaded from Passkey"); | |
throw new Error("No account loaded from Passkey"); | |
} | |
setAccount(account); | |
} catch (e) { | |
handleError(e instanceof Error ? e.message : "Unknown error"); | |
} finally { | |
setLoading(false); | |
} | |
}, [ | |
const handleLoginWithPasskey = useCallback(async () => { | |
try { | |
setLoading(true); | |
const { account } = await loadAccountFromPasskey({ | |
setStatusString, | |
setPreviousPasskeyName, | |
setTurnkeyInfo, | |
}); | |
if (!account) { | |
handleError("No account loaded from Passkey"); | |
return; | |
} | |
setAccount(account); | |
} catch (e) { | |
handleError(e instanceof Error ? e.message : "Unknown error"); | |
} finally { | |
setLoading(false); | |
} | |
}, [ |
const onboardWithPasskey = useCallback(async () => { | ||
try { | ||
setLoading(true); | ||
const account = await onPasskeyCreate({ | ||
passkeyName: inputTextRef.current ?? "", | ||
setStatusString, | ||
setPreviousPasskeyName, | ||
setTurnkeyInfo, | ||
}); | ||
await initXmtpClientFromViemAccount({ | ||
account, | ||
address: account.address, | ||
}); | ||
if (isMissingConverseProfile()) { | ||
router.navigate("OnboardingUserProfile"); | ||
} else if (needToShowNotificationsPermissions()) { | ||
router.navigate("OnboardingNotifications"); | ||
} else { | ||
setAuthStatus("signedIn"); | ||
} | ||
} catch (e) { | ||
handleError(e instanceof Error ? e.message : "Unknown error"); | ||
} finally { | ||
setLoading(false); | ||
} | ||
}, [ | ||
setLoading, | ||
setStatusString, | ||
setPreviousPasskeyName, | ||
setTurnkeyInfo, | ||
router, | ||
handleError, | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Extract common onboarding logic to avoid duplication.
The onboardWithPasskey
function duplicates logic from handleCreatePasskey
and createXmtpClientFromAccount
.
Consider extracting the common logic:
+const handleNavigation = useCallback(() => {
+ if (isMissingConverseProfile()) {
+ router.navigate("OnboardingUserProfile");
+ } else if (needToShowNotificationsPermissions()) {
+ router.navigate("OnboardingNotifications");
+ } else {
+ setAuthStatus("signedIn");
+ }
+}, [router]);
const onboardWithPasskey = useCallback(async () => {
try {
setLoading(true);
- const account = await onPasskeyCreate({
- passkeyName: inputTextRef.current ?? "",
- setStatusString,
- setPreviousPasskeyName,
- setTurnkeyInfo,
- });
+ const account = await handleCreatePasskey();
+ if (!account) return;
await initXmtpClientFromViemAccount({
account,
address: account.address,
});
- if (isMissingConverseProfile()) {
- router.navigate("OnboardingUserProfile");
- } else if (needToShowNotificationsPermissions()) {
- router.navigate("OnboardingNotifications");
- } else {
- setAuthStatus("signedIn");
- }
+ handleNavigation();
} catch (e) {
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (5)
screens/Onboarding/OnboardingPasskeyScreen.tsx (5)
114-132
: 🛠️ Refactor suggestionRemove redundant error handling.
The function throws an error after already handling it with
handleError
, which is redundant.if (!account) { handleError("No account loaded from Passkey"); - throw new Error("No account loaded from Passkey"); return; }
162-192
: 🛠️ Refactor suggestionImprove error logging and string concatenation.
Replace console.log with logger utility and improve error message construction.
- console.log("error creating Xmtp client", err); + logger.error("Error creating Xmtp client:", err); setStatusString(""); handleError( - "Error creating Xmtp client : " + - (err instanceof Error ? err.message : "") + - (typeof err === "string" ? err : "") + `Error creating Xmtp client: ${ + err instanceof Error ? err.message : typeof err === "string" ? err : "Unknown error" + }` );
194-226
: 🛠️ Refactor suggestionExtract common navigation logic.
The navigation logic is duplicated between
createXmtpClientFromAccount
andonboardWithPasskey
.Extract the common logic to a shared function as suggested in the previous review.
246-251
:⚠️ Potential issueRemove sensitive data from UI.
Displaying raw turnkey info in the UI could expose sensitive data.
Consider masking sensitive information as suggested in the previous review.
258-263
: 🛠️ Refactor suggestionAdd accessibility support to TextField.
The text field lacks accessibility props, which could make it difficult for users with disabilities.
Add accessibility props as suggested in the previous review.
🧹 Nitpick comments (3)
screens/Onboarding/OnboardingPasskeyScreen.tsx (2)
71-79
: Consider adding error codes for better tracking.While the error handling is comprehensive, adding error codes would improve error tracking and debugging.
-const handleError = useCallback( +const handleError = useCallback( + (error: string, code?: string) => { + const errorWithCode = code ? `[${code}] ${error}` : error; logger.error(error); - setError(error); + setError(errorWithCode); - captureErrorWithToast(error); + captureErrorWithToast(errorWithCode); }, [setError] );
264-292
: Update button loading states.The buttons should use operation-specific loading states to prevent UI inconsistencies.
<Button text={"Onboard with passkey"} onPress={onboardWithPasskey} - loading={loading} + loading={createLoading} /> <Button text={translate("passkey.createButton")} onPress={handleCreatePasskey} - loading={loading} + loading={createLoading} variant="secondary" />screens/ConversationList.tsx (1)
278-278
: Consider memoizing socials dataThe addition of socials to the useLayoutEffect dependencies could trigger unnecessary header re-renders. Consider:
- Memoizing the socials data if it doesn't change frequently
- Using a ref if the avatar URI doesn't need to be reactive
- const { data: socials } = useProfileSocials(currentAccount!); + const { data: socials } = React.useMemo( + () => useProfileSocials(currentAccount!), + [currentAccount] + );Also applies to: 451-451
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app.json
(1 hunks)screens/ConversationList.tsx
(4 hunks)screens/NewAccount/NewAccountPasskeyScreen.tsx
(1 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app.json
- screens/NewAccount/NewAccountPasskeyScreen.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-with-eas
🔇 Additional comments (4)
screens/Onboarding/OnboardingPasskeyScreen.tsx (2)
1-32
: LGTM! Well-structured component setup.The component is properly structured with appropriate memoization and context provider setup.
34-70
: Consider using operation-specific loading states.The shared loading state could lead to race conditions when multiple operations are triggered in quick succession.
Consider using operation-specific loading states:
+const [createLoading, setCreateLoading] = useState(false); +const [loginLoading, setLoginLoading] = useState(false); +const [addWalletLoading, setAddWalletLoading] = useState(false); +const [xmtpLoading, setXmtpLoading] = useState(false);screens/ConversationList.tsx (2)
62-63
: LGTM! Clean import additionsThe new imports follow the project's conventions and are properly organized.
328-331
: Verify avatar fallback behaviorWhile the Avatar implementation looks good, ensure that it gracefully handles cases where:
- socials data is undefined/null
- getPreferredAvatar returns null/undefined
- the avatar URI is invalid or inaccessible
Let's verify the Avatar component's implementation:
✅ Verification successful
Avatar implementation properly handles all edge cases ✅
The implementation includes comprehensive error handling for:
- undefined/null socials data
- missing or invalid avatar URIs
- fallback to ENS avatars
- display of initials when no avatar is available
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Avatar component implementation for fallback handling ast-grep --pattern 'const Avatar = $_($$$) => { $$$ }' # Check getPreferredAvatar implementation for null handling ast-grep --pattern 'const getPreferredAvatar = $_($$$) => { $$$ }'Length of output: 129
Script:
#!/bin/bash # First, find relevant files echo "=== Files containing Avatar ===" rg -l "Avatar" --type tsx --type ts echo -e "\n=== Avatar component implementation ===" rg "Avatar.*=.*{" --type tsx --type ts -A 10 echo -e "\n=== getPreferredAvatar implementation ===" rg "getPreferredAvatar.*=.*{" --type tsx --type ts -A 10Length of output: 636
Script:
#!/bin/bash # Search for Avatar component echo "=== Avatar Component ===" rg "export.*Avatar" -A 10 echo -e "\n=== Avatar Usage and Props ===" rg "Avatar.*size.*uri" -A 5 echo -e "\n=== getPreferredAvatar Function ===" rg "getPreferredAvatar" -A 5Length of output: 24796
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
utils/api.ts (1)
830-847
:⚠️ Potential issueCritical: Restore App Check headers and add error handling.
- The App Check headers are commented out, which removes a crucial security measure for the wallet creation endpoint.
- The function lacks error handling for potential failures during wallet creation.
Apply this diff to restore the headers and add error handling:
export const createTurnkeySubOrganization = async ( authenticatorParams: TurnkeyAuthenticatorParams ): Promise<{ subOrganizationId: string; walletId: string; address: string; }> => { + try { const { data } = await api.post( "/api/wallet/create-wallet-turnkey", { authenticatorParams, }, { - // headers: await getAppCheckTokenHeaders(), + headers: await getAppCheckTokenHeaders(), } ); return data; + } catch (error) { + logger.error("[API] Failed to create Turnkey wallet", error); + throw new Error( + error instanceof Error + ? error.message + : "Failed to create wallet. Please try again." + ); + } };
🧹 Nitpick comments (2)
screens/Onboarding/OnboardingPasskeyScreen.tsx (2)
26-32
: Add TypeScript types for component props.Consider adding explicit type definitions for better type safety and documentation:
-export const OnboardingPasskeyScreen = memo(function Screen() { +type OnboardingPasskeyScreenProps = Record<string, never>; +export const OnboardingPasskeyScreen = memo(function Screen(_props: OnboardingPasskeyScreenProps) {
71-78
: Enhance error handling with error categories.Consider categorizing errors to provide more specific user feedback:
+type ErrorCategory = 'passkey' | 'network' | 'auth' | 'unknown'; + +const categorizeError = (error: string): ErrorCategory => { + if (error.includes('passkey')) return 'passkey'; + if (error.includes('network')) return 'network'; + if (error.includes('auth')) return 'auth'; + return 'unknown'; +}; + const handleError = useCallback( (error: string) => { logger.error(error); - setError(error); + const category = categorizeError(error); + setError(`[${category}] ${error}`); captureErrorWithToast(error); }, [setError] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
screens/Onboarding/OnboardingPasskeyScreen.tsx
(1 hunks)utils/api.ts
(3 hunks)utils/appCheck.ts
(1 hunks)
🔇 Additional comments (7)
utils/api.ts (1)
273-281
: LGTM! Well-structured header generation.The function properly handles token retrieval and error cases.
screens/Onboarding/OnboardingPasskeyScreen.tsx (6)
122-125
: Remove redundant error handling.The error is both handled and thrown, which is redundant.
204-236
: Extract common onboarding logic to reduce duplication.The
onboardWithPasskey
function duplicates logic fromhandleCreatePasskey
andcreateXmtpClientFromAccount
.
191-199
: Improve error message formatting in createXmtpClientFromAccount.The error handling is inconsistent with other handlers and uses console.log.
256-261
: Remove sensitive data from UI.Displaying raw turnkey info in the UI could expose sensitive data.
268-273
: Add accessibility support to TextField.The text field lacks accessibility props, which could make it difficult for users with disabilities.
274-302
:⚠️ Potential issuePrevent multiple simultaneous operations.
The loading state is shared across all operations which could lead to race conditions.
+const [operationInProgress, setOperationInProgress] = useState<string | null>(null); + +const withOperation = async (operation: string, callback: () => Promise<void>) => { + if (operationInProgress) { + return; // Prevent multiple operations + } + try { + setOperationInProgress(operation); + setLoading(true); + await callback(); + } finally { + setLoading(false); + setOperationInProgress(null); + } +}; <Button text={"Onboard with passkey"} onPress={onboardWithPasskey} - loading={loading} + loading={loading && operationInProgress === 'onboard'} />Likely invalid or redundant comment.
utils/appCheck.ts
Outdated
// const { token } = await appCheck.getLimitedUseToken(); | ||
// const { token } = await appCheck.getToken(); | ||
return token; | ||
return ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: App Check token verification has been bypassed.
Returning an empty string instead of a valid App Check token circumvents a crucial security mechanism. App Check is designed to verify that requests originate from legitimate instances of your application, protecting your backend resources from abuse.
Restore the token retrieval logic:
- // const { token } = await appCheck.getLimitedUseToken();
- // const { token } = await appCheck.getToken();
- return "";
+ const { token } = await appCheck.getToken();
+ return token;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// const { token } = await appCheck.getLimitedUseToken(); | |
// const { token } = await appCheck.getToken(); | |
return token; | |
return ""; | |
const { token } = await appCheck.getToken(); | |
return token; |
(state) => state.setPreviousPasskeyName | ||
); | ||
|
||
const inputTextRef = useRef<string>(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add input validation for passkey name.
The current implementation allows empty strings. Consider adding validation and minimum length requirements for the passkey name.
-const inputTextRef = useRef<string>("");
+const inputTextRef = useRef<string>("");
+const validatePasskeyName = (name: string): boolean => {
+ return name.length >= 3; // Minimum 3 characters
+};
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (4)
utils/appCheck.ts (1)
17-19
:⚠️ Potential issueCritical: App Check token verification has been bypassed
Returning a hardcoded string instead of a valid App Check token circumvents a crucial security mechanism. App Check is designed to verify that requests originate from legitimate instances of your application, protecting your backend resources from abuse.
Let's verify the backend dependencies on App Check:
#!/bin/bash # Check backend middleware for App Check dependencies rg -A 10 "app-?check" --type ts # Check for protected endpoints ast-grep --pattern 'app.use($$$appCheck$$$)'Restore the token retrieval logic:
- // const { token } = await appCheck.getLimitedUseToken(); - // const { token } = await appCheck.getToken(); - return "Placeholder"; + const { token } = await appCheck.getToken(); + return token;screens/NewAccount/NewAccountPasskeyScreen.tsx (3)
174-181
:⚠️ Potential issueImprove error handling in createXmtpClientFromAccount
The error handling concatenates different error types unsafely and uses console.log instead of the logger.
- console.log("error creating Xmtp client", err); + logger.error("Error creating Xmtp client:", err); setStatusString(""); - handleError( - "Error creating Xmtp client : " + - (err instanceof Error ? err.message : "") + - (typeof err === "string" ? err : "") - ); + handleError( + `Error creating Xmtp client: ${ + err instanceof Error ? err.message : + typeof err === "string" ? err : + "Unknown error" + }` + );
279-284
:⚠️ Potential issueSecurity: Remove sensitive information from UI
Displaying turnkey information in the UI could pose a security risk if screenshots are taken or if the screen is visible to others.
- {turnkeyInfo && ( - <Text preset="bodyBold"> - Turnkey info: - <Text preset="body">{JSON.stringify(turnkeyInfo)}</Text> - </Text> - )} + {turnkeyInfo && ( + <Text preset="bodyBold"> + Passkey configured successfully + </Text> + )}
291-296
:⚠️ Potential issueAdd input validation for passkey name
The TextField for passkey name lacks validation and error handling.
<TextField placeholder="Enter your passkey" + error={inputTextRef.current?.length < 3 ? "Passkey name must be at least 3 characters" : undefined} onChangeText={(text) => { inputTextRef.current = text; }} + onBlur={() => { + if (!inputTextRef.current) { + setError("Passkey name is required"); + } + }} />
🧹 Nitpick comments (1)
screens/NewAccount/NewAccountPasskeyScreen.tsx (1)
19-20
: Consider consolidating error handling utilitiesThe error handling utilities (
captureErrorWithToast
andlogger
) could be combined into a single error handling service for better maintainability.-import { captureErrorWithToast } from "@/utils/capture-error"; -import logger from "@/utils/logger"; +import { ErrorService } from "@/services/error";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
screens/NewAccount/NewAccountPasskeyScreen.tsx
(1 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx
(1 hunks)utils/appCheck.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- screens/Onboarding/OnboardingPasskeyScreen.tsx
🔇 Additional comments (1)
screens/NewAccount/NewAccountPasskeyScreen.tsx (1)
297-331
: 🛠️ Refactor suggestionImplement separate loading states for different operations
Currently, all buttons share the same loading state, which could lead to unclear UI feedback when multiple operations are in progress.
+ const [createLoading, setCreateLoading] = useState(false); + const [loginLoading, setLoginLoading] = useState(false); + const [addWalletLoading, setAddWalletLoading] = useState(false); + const [xmtpLoading, setXmtpLoading] = useState(false); // Update button loading props <Button text={"Onboard with passkey"} onPress={onboardWithPasskey} - loading={loading} + loading={createLoading} /> <Button text={"Add new wallet to existing passkey"} onPress={addWalletToExistingPasskey} - loading={loading} + loading={addWalletLoading} />Likely invalid or redundant comment.
const handleError = useCallback( | ||
(error: string) => { | ||
logger.error(error); | ||
setError(error); | ||
captureErrorWithToast(error); | ||
}, | ||
[setError] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance error handling with better type safety and messages
The current error handling implementation could be improved with better type safety and more descriptive error messages.
- const handleError = useCallback(
- (error: string) => {
+ const handleError = useCallback(
+ (error: Error | string) => {
+ const errorMessage = error instanceof Error
+ ? `${error.name}: ${error.message}`
+ : error;
- logger.error(error);
- setError(error);
- captureErrorWithToast(error);
+ logger.error(errorMessage);
+ setError(errorMessage);
+ captureErrorWithToast(errorMessage);
},
- [setError]
+ [setError]
);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const handleError = useCallback( | |
(error: string) => { | |
logger.error(error); | |
setError(error); | |
captureErrorWithToast(error); | |
}, | |
[setError] | |
); | |
const handleError = useCallback( | |
(error: Error | string) => { | |
const errorMessage = error instanceof Error | |
? `${error.name}: ${error.message}` | |
: error; | |
logger.error(errorMessage); | |
setError(errorMessage); | |
captureErrorWithToast(errorMessage); | |
}, | |
[setError] | |
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
theme/useAppTheme.ts (1)
154-154
: Remove commented type definition.Since
navTheme
has been removed from the implementation, the commented type definition should also be removed for clarity.- // navTheme: typeof DefaultTheme;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
screens/Main.tsx
(1 hunks)theme/useAppTheme.ts
(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- screens/Main.tsx
🔇 Additional comments (2)
theme/useAppTheme.ts (2)
212-215
: LGTM! Comprehensive debug logging.The debug logging changes are well-structured and include all necessary information for debugging theme-related issues. The dependency array correctly includes all referenced variables.
Line range hint
180-190
: Verify impact of theme management changes.The shift to system color scheme-based theme management is a significant architectural change. While it improves consistency, we should verify that existing theme-dependent components continue to work as expected.
✅ Verification successful
Theme management changes are architecturally sound
The shift to system color scheme-based theme management maintains backward compatibility while improving consistency:
- Existing components continue to access theme properties through the same API
- Fallback mechanism ensures consistent theme values
- Theme override capability is preserved
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for components using theme-related hooks echo "Components using useAppTheme:" rg "useAppTheme" --type ts --type tsx -l echo "\nComponents using useTheme:" rg "useTheme" --type ts --type tsx -l echo "\nComponents directly accessing theme context:" rg "ThemeContext" --type ts --type tsx -lLength of output: 443
Script:
#!/bin/bash echo "=== Components using useAppTheme ===" rg "useAppTheme" -A 2 -B 2 echo -e "\n=== Components using useTheme ===" rg "useTheme" -A 2 -B 2 echo -e "\n=== Components using ThemeContext ===" rg "ThemeContext" -A 2 -B 2Length of output: 66351
() => overrideTheme || (controlledTheme ? "dark" : "light"), | ||
[overrideTheme, controlledTheme] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix incorrect theme context determination logic.
The current condition controlledTheme ? "dark" : "light"
will set the theme to "dark" for both "dark" and "light" system color schemes, as both are truthy values. This could lead to unexpected behavior.
- () => overrideTheme || (controlledTheme ? "dark" : "light"),
+ () => overrideTheme || controlledTheme || "light",
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
() => overrideTheme || (controlledTheme ? "dark" : "light"), | |
[overrideTheme, controlledTheme] | |
() => overrideTheme || controlledTheme || "light", | |
[overrideTheme, controlledTheme] |
5c9ba52
to
431056e
Compare
Added better support for login with passkeys
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
Removed appcheck
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
79713b2
to
6a1cb65
Compare
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
ios/ConverseNotificationExtension/Xmtp/Messages.swift (1)
Line range hint
140-165
: Remove force unwrap to prevent crashes.The forced try operator
try!
could cause a crash iffindConversationByTopic
fails. Replace it with proper error handling.if (isV3MessageTopic(topic: envelope.contentTopic)) { - if let conversation = try! await xmtpClient.findConversationByTopic(topic: envelope.contentTopic) { + do { + guard let conversation = try await xmtpClient.findConversationByTopic(topic: envelope.contentTopic) else { + sentryTrackMessage(message: "NOTIFICATION_GROUP_NOT_FOUND", extras: ["envelope": envelope]) + return nil + }
♻️ Duplicate comments (6)
utils/api.ts (2)
843-843
:⚠️ Potential issueRe-enable App Check token headers for security.
The App Check token headers are commented out, which exposes the endpoint to unauthorized access. This is a critical security concern.
Apply this diff to fix the issue:
- // headers: await getAppCheckTokenHeaders(), + headers: await getAppCheckTokenHeaders(),
830-847
:⚠️ Potential issueAdd error handling for wallet creation failures.
The function should handle potential errors during wallet creation and provide meaningful error messages.
Apply this diff to add error handling:
export const createTurnkeySubOrganization = async ( authenticatorParams: TurnkeyAuthenticatorParams ): Promise<{ subOrganizationId: string; walletId: string; address: string; }> => { + try { const { data } = await api.post( "/api/wallet/create-wallet-turnkey", { authenticatorParams, }, { headers: await getAppCheckTokenHeaders(), } ); return data; + } catch (error) { + logger.error("[API] Failed to create Turnkey wallet", error); + throw new Error( + error instanceof Error + ? error.message + : "Failed to create wallet. Please try again." + ); + } };theme/useAppTheme.ts (2)
189-190
:⚠️ Potential issueFix incorrect theme context determination logic.
The condition
controlledTheme ? "dark" : "light"
will incorrectly set the theme to "dark" for both "dark" and "light" system color schemes, as both are truthy values.- () => overrideTheme || (controlledTheme ? "dark" : "light"), + () => overrideTheme || controlledTheme || "light",
212-215
:⚠️ Potential issueFix incorrect debug logging condition.
The condition
!__DEV__
means this logging runs in production but not in development, which is likely the opposite of what's intended.- if (!__DEV__) { + if (__DEV__) {screens/Onboarding/OnboardingPasskeyScreen.tsx (1)
261-266
:⚠️ Potential issueRemove sensitive data from UI.
Displaying raw turnkey info in the UI could expose sensitive data.
Apply this diff to mask sensitive information:
{turnkeyInfo && ( <Text preset="bodyBold"> - Turnkey info: - <Text preset="body">{JSON.stringify(turnkeyInfo)}</Text> + Passkey status: + <Text preset="body">Connected</Text> </Text> )}i18n/translations/en.ts (1)
55-60
: 🛠️ Refactor suggestionAdd error messages for passkey operations.
The passkey translations should include error messages for common failure scenarios.
Add these translations:
passkey: { title: "Create Passkey", add_account_title: "Add account by passkey", subtitle: "Create a passkey to connect to Converse", createButton: "Create Passkey", + errors: { + creation_failed: "Failed to create passkey", + login_failed: "Failed to login with passkey", + invalid_name: "Invalid passkey name", + already_exists: "A passkey with this name already exists", + }, },
🧹 Nitpick comments (10)
queries/useConversationListQuery.ts (1)
182-190
: Consider optimizing the sync operations for better performance.The function performs two sync operations (
sync()
andsyncAllConversations()
) before fetching the list. Consider:
- Investigating if both sync operations are necessary or if they can be combined
- Adding more granular performance logging to measure each sync operation separately
const beforeSync = new Date().getTime(); if (includeSync) { + const syncStart = new Date().getTime(); await client.conversations.sync(); + const syncEnd = new Date().getTime(); + logger.debug( + `[ConversationListQuery] Initial sync took ${ + (syncEnd - syncStart) / 1000 + } sec` + ); + const syncAllStart = new Date().getTime(); await client.conversations.syncAllConversations(); + const syncAllEnd = new Date().getTime(); + logger.debug( + `[ConversationListQuery] Sync all conversations took ${ + (syncAllEnd - syncAllStart) / 1000 + } sec` + ); } const afterSync = new Date().getTime();ios/ConverseNotificationExtension/Spam.swift (1)
Line range hint
155-212
: Consider caching network consent checks to improve performance.The function makes multiple async calls to
client.preferences
which could impact performance. Consider caching these results or batching the checks.func computeSpamScoreV3Message(client: XMTP.Client, conversation: XMTP.Conversation, decodedMessage: Message, apiURI: String?) async -> Double { do { + // Cache preferences to avoid multiple network calls + let conversationState = try await client.preferences.conversationState(conversationId: conversation.id) + let senderInboxState = try await client.preferences.inboxIdState(inboxId: decodedMessage.senderInboxId) - let groupDenied = try await client.preferences.conversationState(conversationId: conversation.id) == .denied + let groupDenied = conversationState == .denied if groupDenied { return 1 } - let senderDenied = try await client.preferences.inboxIdState(inboxId: senderInboxId) == .denied + let senderDenied = senderInboxState == .denied if senderDenied { return 1 } // ... similar changes for other checksios/ConverseNotificationExtension/Xmtp/Messages.swift (1)
Line range hint
166-251
: Refactor content type handling to reduce complexity.The current implementation uses string prefix checks which are fragile and hard to maintain. Consider using an enum-based approach.
+enum ContentType { + case text + case remoteAttachment + case transactionReference + case reaction + case readReceipt + case unknown + + static func from(_ type: ContentTypeID) -> ContentType { + switch type.typeID { + case let id where id.starts(with: "text"): return .text + case let id where id.starts(with: "remoteStaticAttachment"): return .remoteAttachment + // ... other cases + default: return .unknown + } + } +} func handleMessageByContentType(decodedMessage: Message, xmtpClient: XMTP.Client) -> (content: String?, senderAddress: String?, forceIgnore: Bool, id: String?) { guard let contentType = try? decodedMessage.encodedContent.type else { return (nil, nil, true, nil) } - switch contentType { - case let type where type.typeID.starts(with: "text"): + switch ContentType.from(contentType) { + case .text: // ... handle text - case let type where type.typeID.starts(with: "remoteStaticAttachment"): + case .remoteAttachment: // ... handle attachmentutils/passkeys/turnkey-clients.ts (2)
5-5
: Add type safety to client map initializationConsider using a more type-safe initialization to prevent potential runtime errors.
-export const turnkeyClientMap: Record<string, TurnkeyClient> = {}; +export const turnkeyClientMap = new Map<string, TurnkeyClient>();
20-20
: Simplify boolean checkRemove redundant double negation.
- if (!!client) { + if (client) {🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
utils/passkeys/add-wallet-to-passkey.ts (1)
34-39
: Add documentation for wallet creation parametersAdd JSDoc comments to explain the purpose and requirements of each parameter.
+ /** + * CURVE_SECP256K1: Standard curve used for Ethereum addresses + * PATH_FORMAT_BIP32: Standard derivation path format + * m/44'/60'/0'/0/0: Standard Ethereum derivation path + * ADDRESS_FORMAT_ETHEREUM: Format for Ethereum addresses + */ { curve: "CURVE_SECP256K1", pathFormat: "PATH_FORMAT_BIP32", path: "m/44'/60'/0'/0/0", addressFormat: "ADDRESS_FORMAT_ETHEREUM", },utils/passkeys/load-client-from-passkey.ts (1)
43-46
: Enhance embedded wallet search logicThe current implementation uses a simple name match which could be fragile. Consider using a more robust identifier or maintaining a list of valid wallet types.
- const embeddedWallet = getWallets.wallets.find( - (wallet) => wallet.walletName === "Embedded Wallet" - ); + const VALID_WALLET_TYPES = ['Embedded Wallet']; + const embeddedWallet = getWallets.wallets.find( + (wallet) => VALID_WALLET_TYPES.includes(wallet.walletName) + );utils/passkeys/create-passkey.ts (1)
70-73
: Move authenticator parameters to configurationThe authenticator name and RP name are hardcoded. These should be configurable values.
- authenticatorName: "End-User Passkey", - rp: { - id: RPID, - name: "Passkey App", - }, + authenticatorName: config.passkey.authenticatorName, + rp: { + id: RPID, + name: config.passkey.rpName, + },features/onboarding/passkey/passkeyAuthStore.tsx (1)
63-63
: Add validation for turnkeyInfo updatesThe setTurnkeyInfo function accepts undefined values without validation, which could lead to inconsistent state.
- setTurnkeyInfo: (turnkeyInfo) => set({ turnkeyInfo }), + setTurnkeyInfo: (turnkeyInfo) => + set((state) => { + if (turnkeyInfo && (!turnkeyInfo.subOrganizationId || !turnkeyInfo.walletId)) { + console.warn('Invalid turnkey info provided'); + return state; + } + return { turnkeyInfo }; + }),screens/NewAccount/NewAccountPasskeyScreen.tsx (1)
219-252
: Extract common XMTP client initialization logic.The XMTP client initialization logic is duplicated across multiple functions.
Consider extracting the common logic into a shared function:
+const initializeXmtpAndNavigate = async (account: Account) => { + await initXmtpClientFromViemAccount({ + account, + address: account.address, + }); + if (isMissingConverseProfile()) { + router.navigate("NewAccountUserProfile"); + } else { + router.popTo("Chats"); + } +}; const addWalletToExistingPasskey = useCallback(async () => { try { // ... existing code ... - await initXmtpClientFromViemAccount({ - account, - address: account.address, - }); - if (isMissingConverseProfile()) { - router.navigate("NewAccountUserProfile"); - } else { - router.popTo("Chats"); - } + await initializeXmtpAndNavigate(account); } catch (e) { // ... error handling ... } }, [/* ... dependencies ... */]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lock
is excluded by!**/*.lock
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (40)
.github/workflows/build-app-release.yml
(2 hunks).github/workflows/build-internal.yml
(1 hunks)app.json
(1 hunks)components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx
(1 hunks)components/Onboarding/init-xmtp-client.ts
(2 hunks)config.ts
(4 hunks)features/onboarding/passkey/passkeyAuthStore.tsx
(1 hunks)i18n/translations/en.ts
(2 hunks)ios/Converse/Converse.entitlements
(1 hunks)ios/ConverseNotificationExtension/Spam.swift
(2 hunks)ios/ConverseNotificationExtension/Xmtp/Messages.swift
(9 hunks)ios/Podfile
(1 hunks)navigation/OnboardingNavigator.tsx
(3 hunks)package.json
(3 hunks)queries/useConversationListQuery.ts
(1 hunks)screens/Main.tsx
(1 hunks)screens/Navigation/Navigation.tsx
(7 hunks)screens/NewAccount/NewAccountEphemeralScreen.tsx
(1 hunks)screens/NewAccount/NewAccountPasskeyScreen.tsx
(1 hunks)screens/NewAccount/NewAccountScreen.tsx
(2 hunks)screens/Onboarding/OnboardingEphemeralScreen.tsx
(1 hunks)screens/Onboarding/OnboardingGetStartedScreen.tsx
(2 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx
(1 hunks)scripts/build/ios/preview.js
(1 hunks)scripts/build/ios/production.js
(1 hunks)theme/useAppTheme.ts
(4 hunks)utils/api.ts
(3 hunks)utils/appCheck.ts
(1 hunks)utils/navigation.ts
(1 hunks)utils/passkeys/add-wallet-to-passkey.ts
(1 hunks)utils/passkeys/create-passkey.ts
(1 hunks)utils/passkeys/create-suborganization.ts
(1 hunks)utils/passkeys/create-turnkey-account.ts
(1 hunks)utils/passkeys/load-client-from-passkey.ts
(1 hunks)utils/passkeys/passkeys.constants.ts
(1 hunks)utils/passkeys/passkeys.interfaces.ts
(1 hunks)utils/passkeys/turnkey-clients.ts
(1 hunks)utils/xmtpRN/conversations.ts
(4 hunks)utils/xmtpRN/signIn.ts
(2 hunks)utils/xmtpRN/signer.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- utils/passkeys/passkeys.constants.ts
- utils/navigation.ts
- .github/workflows/build-internal.yml
- screens/Onboarding/OnboardingEphemeralScreen.tsx
- screens/Main.tsx
- app.json
- utils/passkeys/create-suborganization.ts
- utils/passkeys/passkeys.interfaces.ts
- config.ts
- components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx
- scripts/build/ios/production.js
- ios/Converse/Converse.entitlements
- utils/xmtpRN/signer.ts
- screens/NewAccount/NewAccountScreen.tsx
- utils/appCheck.ts
- scripts/build/ios/preview.js
- utils/passkeys/create-turnkey-account.ts
- screens/NewAccount/NewAccountEphemeralScreen.tsx
- navigation/OnboardingNavigator.tsx
- screens/Navigation/Navigation.tsx
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
utils/passkeys/turnkey-clients.ts
[error] 20-20: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: performance-test
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (24)
utils/api.ts (1)
273-281
: LGTM: Well-structured utility function.The
getAppCheckTokenHeaders
function is well-implemented with proper error handling and clear return type.queries/useConversationListQuery.ts (1)
182-190
: LGTM! The conversation list options look good.The options passed to
client.conversations.list
are comprehensive and well-structured, including all necessary fields for the conversation list.theme/useAppTheme.ts (2)
154-154
: LGTM: Clean removal of unused type.The removal of
navTheme
aligns well with the migration to system color scheme-based theming.
180-180
: LGTM: Good use of useColorScheme hook.The introduction of
controlledTheme
usinguseColorScheme()
is a clean way to handle system theme preferences..github/workflows/build-app-release.yml (3)
51-51
: Ensure consistent environment variable handlingThe new environment variable follows the established pattern and is correctly added to both Android and iOS builds.
Also applies to: 95-95
Line range hint
9-14
: Verify the impact of removing scheduled releasesThe removal of scheduled triggers (Tuesday/Thursday) significantly changes the release automation. Please confirm:
- Is this aligned with the team's current release strategy?
- Are there alternative mechanisms in place for regular releases?
- Has this been communicated to stakeholders?
✅ Verification successful
Scheduled release removal appears safe and intentional ✅
The workflow maintains full release capabilities through manual triggers (
workflow_dispatch
), and the change aligns with the comment about matching the current release schedule. The complete release process, including builds, version increments, and release notes generation, remains intact.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any other scheduled workflows that might be handling releases fd -e yml -e yaml . .github/workflows --exec grep -l "cron:"Length of output: 100
Script:
#!/bin/bash # Search for any release-related workflows fd -e yml -e yaml . .github/workflows --exec grep -l -i "release" # Show the complete content of build-app-release.yml to examine other triggers cat .github/workflows/build-app-release.ymlLength of output: 6394
51-51
: Review security implications of TURNKEY_ORG configurationThe addition of
EXPO_PUBLIC_TURNKEY_ORG
to the production environment requires security review:
- The
EXPO_PUBLIC_
prefix indicates this will be publicly accessible in the client-side code- Verify if exposing the Turnkey organization identifier poses any security risks
Also applies to: 95-95
✅ Verification successful
Organization ID is safe for public exposure
The
EXPO_PUBLIC_TURNKEY_ORG
appears to be a public identifier used for passkey client configuration, similar to OAuth client IDs. The code usage shows it's specifically designed for client-side functionality and doesn't expose sensitive operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any security-related comments or documentation about Turnkey configuration rg -i "turnkey.*security|security.*turnkey" --type mdLength of output: 55
Script:
#!/bin/bash # Search for Turnkey documentation and configuration echo "=== Searching for Turnkey documentation ===" rg -i "turnkey" --type md echo -e "\n=== Finding TURNKEY_ORG usage ===" rg "TURNKEY_ORG|turnkeyOrg" echo -e "\n=== Looking for similar EXPO_PUBLIC patterns ===" rg "EXPO_PUBLIC_" .env* 2>/dev/null || true rg "EXPO_PUBLIC_" --type yamlLength of output: 556
ios/ConverseNotificationExtension/Xmtp/Messages.swift (1)
287-291
: LGTM!The function is well-structured, handles optionals correctly, and has a clear purpose.
utils/xmtpRN/conversations.ts (2)
35-35
: LGTM! Simplified function signatureThe removal of the
order
parameter streamlines the conversation listing functionality while maintaining core behavior.Also applies to: 49-49
63-63
: LGTM! Consistent parameter updatesThe changes to remove the
order
parameter are consistently applied across related functions.Also applies to: 71-71
utils/passkeys/turnkey-clients.ts (3)
7-9
: Add error handling for missing clientsThe getter should handle the case when a client doesn't exist for the given
subOrgId
.export const getTurnkeyClient = (subOrgId: string) => { + if (!turnkeyClientMap[subOrgId]) { + throw new Error(`No TurnkeyClient found for subOrgId: ${subOrgId}`); + } return turnkeyClientMap[subOrgId]; };
11-16
: Add input validation for setTurnkeyClientThe setter should validate its inputs to prevent potential issues.
export const setTurnkeyClient = ( subOrgId: string, turnkeyClient: TurnkeyClient ) => { + if (!subOrgId?.trim()) { + throw new Error('subOrgId is required'); + } + if (!turnkeyClient) { + throw new Error('turnkeyClient is required'); + } turnkeyClientMap[subOrgId] = turnkeyClient; };
28-28
: Move API URL to environment configurationHardcoding the API URL is not a good practice. Move it to environment configuration.
- client = new TurnkeyClient({ baseUrl: "https://api.turnkey.com" }, stamper); + client = new TurnkeyClient({ baseUrl: config.turnkeyApiUrl }, stamper);utils/passkeys/add-wallet-to-passkey.ts (2)
44-47
: Add error handling for wallet creation responseThe wallet creation response is not properly validated before accessing nested properties.
- const address = res.activity.result.createWalletResult?.addresses[0]; + const addresses = res.activity?.result?.createWalletResult?.addresses; + if (!addresses?.length) { + throw new Error("Wallet creation failed: no addresses returned"); + } + const address = addresses[0];
23-23
: Move API URL to environment configurationHardcoding the API URL makes it difficult to switch between environments and poses a security risk if the URL changes.
- client = new TurnkeyClient({ baseUrl: "https://api.turnkey.com" }, stamper); + client = new TurnkeyClient({ baseUrl: process.env.TURNKEY_API_URL }, stamper);utils/passkeys/create-passkey.ts (1)
58-61
:⚠️ Potential issueReturn early if passkeys are not supported
The function continues execution even when passkeys are not supported, which could lead to errors.
if (!isSupported()) { alert("Passkeys are not supported on this device"); + return; }
Likely invalid or redundant comment.
features/onboarding/passkey/passkeyAuthStore.tsx (1)
66-66
: 🛠️ Refactor suggestionPreserve initial props in reset function
The reset function only uses DEFAULT_PROPS, losing any initial props that were set during initialization.
- reset: () => set(DEFAULT_PROPS), + reset: () => set({ ...DEFAULT_PROPS, ...initProps }),Likely invalid or redundant comment.
utils/xmtpRN/signIn.ts (1)
72-120
: 🛠️ Refactor suggestionReduce code duplication with base function
The
createXmtpClientFromViemAccount
function duplicates most of the logic fromcreateXmtpClientFromSigner
. Consider extracting common functionality into a shared helper function.+async function createXmtpClientBase<T>( + signerOrAccount: T, + createSigner: (input: T) => XmtpSigner, + onInstallationRevoked: () => Promise<void>, + preAuthenticateToInboxCallback?: () => Promise<void> +) { + const tempDirectory = await createTemporaryDirectory(); + const dbEncryptionKey = await getDbEncryptionKey(); + + const options = { + env, + enableV3: true, + dbDirectory: tempDirectory, + dbEncryptionKey, + }; + + const address = typeof signerOrAccount === 'object' && 'address' in signerOrAccount + ? signerOrAccount.address + : await (signerOrAccount as Signer).getAddress(); + + const inboxId = await getInboxId(address); + + await copyDatabasesToTemporaryDirectory(tempDirectory, inboxId); + + logger.debug("Instantiating client from signer"); + + const client = await Client.create(createSigner(signerOrAccount), { + ...options, + preAuthenticateToInboxCallback, + }); + + if (client.inboxId !== inboxId) { + throw new Error("Inbox ids don't match"); + } + + const installationValid = await isClientInstallationValid(client); + if (!installationValid) { + await client.dropLocalDatabaseConnection(); + await deleteLibXmtpDatabaseForInboxId(inboxId); + onInstallationRevoked(); + return; + } + + logger.debug("Instantiated client from signer"); + await client.dropLocalDatabaseConnection(); + await moveTemporaryDatabasesToDatabaseDirectory(tempDirectory, client.inboxId); + logger.debug("Dropped client databases"); +} export const createXmtpClientFromSigner = async ( signer: Signer, onInstallationRevoked: () => Promise<void>, preAuthenticateToInboxCallback?: () => Promise<void> -) => { - const tempDirectory = await createTemporaryDirectory(); - // ... rest of the implementation +) => createXmtpClientBase( + signer, + (s) => ethersSignerToXmtpSigner(s), + onInstallationRevoked, + preAuthenticateToInboxCallback +); export const createXmtpClientFromViemAccount = async ( account: LocalAccount, onInstallationRevoked: () => Promise<void>, preAuthenticateToInboxCallback?: () => Promise<void> -) => { - const tempDirectory = await createTemporaryDirectory(); - // ... rest of the implementation +) => createXmtpClientBase( + account, + (a) => viemAccountToXmtpSigner(a), + onInstallationRevoked, + preAuthenticateToInboxCallback +);Likely invalid or redundant comment.
components/Onboarding/init-xmtp-client.ts (2)
65-67
:⚠️ Potential issueFix incorrect error message
The error message refers to "signer" but the function checks for "account".
if (!account || !address) { - throw new Error("No signer or address"); + throw new Error("No account or address"); }Likely invalid or redundant comment.
56-87
: 🛠️ Refactor suggestionReduce code duplication and improve error handling
The
initXmtpClientFromViemAccount
function largely duplicatesinitXmtpClient
. Consider refactoring to reduce duplication and improve error handling.+type BaseInitArgs = { + address: string; + privyAccountId?: string; + isEphemeral?: boolean; + pkPath?: string; +}; + +type InitXmtpArgs = BaseInitArgs & { + signer?: Signer; + account?: LocalAccount; +}; + +async function initXmtpBase( + createClient: () => Promise<void>, + args: BaseInitArgs +) { + try { + await createClient(); + await connectWithAddress(args); + } catch (e) { + await logoutAccount(args.address, false, true, () => {}); + logger.error(e); + throw e; + } +} + -export async function initXmtpClientFromViemAccount(args: { - account: LocalAccount; - address: string; - privyAccountId?: string; - isEphemeral?: boolean; - pkPath?: string; -}) { +export async function initXmtpClientFromViemAccount( + args: InitXmtpArgs & Required<Pick<InitXmtpArgs, "account">> +) { const { account, address, ...restArgs } = args; if (!account || !address) { throw new Error("No account or address"); } - try { - await createXmtpClientFromViemAccount(account, async () => { + return initXmtpBase( + () => createXmtpClientFromViemAccount(account, async () => { await awaitableAlert( translate("current_installation_revoked"), translate("current_installation_revoked_description") ); throw new Error("Current installation revoked"); - }); - await connectWithAddress({ + }), + { address, ...restArgs, - }); - } catch (e) { - await logoutAccount(address, false, true, () => {}); - logger.error(e); - throw e; - } + } + ); }Likely invalid or redundant comment.
screens/Onboarding/OnboardingGetStartedScreen.tsx (1)
62-66
: LGTM! Passkey option well integrated.The passkey connection option is cleanly integrated into the existing TableView items array, maintaining consistency with other connection methods.
screens/Onboarding/OnboardingPasskeyScreen.tsx (3)
119-130
:⚠️ Potential issueRemove redundant error handling.
The function throws an error after already handling it with
handleError
, which could cause confusion in error handling flow.Apply this diff to fix the redundant error handling:
if (!account) { handleError("No account loaded from Passkey"); - throw new Error("No account loaded from Passkey"); return; }
Likely invalid or redundant comment.
174-182
: 🛠️ Refactor suggestionImprove error handling in createXmtpClientFromAccount.
The error handling uses string concatenation and console.log, which is inconsistent with the app's error handling patterns.
Apply this diff to improve error handling:
- console.log("error creating Xmtp client", err); setStatusString(""); - handleError( - "Error creating Xmtp client : " + - (err instanceof Error ? err.message : "") + - (typeof err === "string" ? err : "") - ); + handleError( + `Error creating Xmtp client: ${ + err instanceof Error ? err.message : + typeof err === "string" ? err : + "Unknown error" + }` + );Likely invalid or redundant comment.
291-296
: 🛠️ Refactor suggestionAdd input validation for passkey name.
The TextField lacks validation for the passkey name, which could lead to invalid or empty passkey names.
Apply this diff to add input validation:
<TextField placeholder="Enter your passkey" + error={ + inputTextRef.current?.length > 0 && + inputTextRef.current.length < 3 + ? "Passkey name must be at least 3 characters" + : undefined + } onChangeText={(text) => { inputTextRef.current = text; }} + onBlur={() => { + if (!inputTextRef.current) { + setError("Passkey name is required"); + } + }} />Likely invalid or redundant comment.
guard let messageContentType = try? decodedMessage.encodedContent.type else { | ||
return 1 | ||
} | ||
let contentType = getContentTypeString(type: messageContentType) | ||
|
||
|
||
let messageContent = String(data: decodedMessage.encodedContent.content, encoding: .utf8) | ||
guard let content = try? decodedMessage.encodedContent.content else { | ||
return 1 | ||
} | ||
|
||
let messageContent = String(data:content, encoding: .utf8) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling for message content extraction.
The current implementation silently returns a spam score of 1 when content extraction fails. Consider logging the specific error for debugging.
- guard let messageContentType = try? decodedMessage.encodedContent.type else {
+ guard let messageContentType = try? decodedMessage.encodedContent.type else {
+ sentryTrackError(error: NSError(domain: "", code: -1), extras: ["message": "Failed to extract message content type"])
return 1
}
- guard let content = try? decodedMessage.encodedContent.content else {
+ guard let content = try? decodedMessage.encodedContent.content else {
+ sentryTrackError(error: NSError(domain: "", code: -1), extras: ["message": "Failed to extract message content"])
return 1
}
Committable suggestion skipped: line range outside the PR's diff.
@@ -14,7 +14,7 @@ install! 'cocoapods', | |||
|
|||
# Version must match version from XMTP Podspec (matching @xmtp/react-native-sdk from package.json) | |||
# https://github.com/xmtp/xmtp-react-native/blob/v2.6.2/ios/XMTPReactNative.podspec#L29 | |||
$xmtpVersion = '3.0.21' | |||
$xmtpVersion = '3.0.22' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
There's a significant version mismatch between the XMTP dependencies:
- iOS Podfile:
3.0.22
- React Native SDK:
^3.1.6
Consider updating the Podfile's $xmtpVersion
to at least 3.1.0
to maintain version alignment with the React Native SDK. Using an older native SDK version with a newer React Native wrapper could lead to compatibility issues.
🔗 Analysis chain
Verify XMTP version compatibility
The XMTP version has been updated from '3.0.21' to '3.0.22'. Please ensure this version matches the version from XMTP Podspec and is compatible with @xmtp/react-native-sdk
from package.json.
Run this script to check version compatibility:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check XMTP version compatibility between Podfile and package.json
# Get XMTP version from package.json
echo "XMTP version in package.json:"
cat package.json | jq '.dependencies["@xmtp/react-native-sdk"]'
# Get XMTP version from Podspec
echo -e "\nXMTP version in Podspec:"
gh api repos/xmtp/xmtp-react-native/contents/ios/XMTPReactNative.podspec | jq -r '.content' | base64 -d | grep -A 1 'spec.version'
Length of output: 435
Script:
#!/bin/bash
# Search for documentation about XMTP version compatibility
echo "Checking documentation for version compatibility info:"
fd -e md -e txt | xargs rg -i "version|compatibility|xmtp" -A 2
# Also check package.json in full for any additional XMTP related dependencies
echo -e "\nChecking package.json for XMTP related entries:"
cat package.json | jq 'del(.dependencies[] | select(. | test("@xmtp") | not)) | .dependencies'
Length of output: 3146
rpId: RPID, | ||
}); | ||
const client = new TurnkeyClient( | ||
{ baseUrl: "https://api.turnkey.com" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Move base URL to configuration
The Turnkey API base URL is hardcoded. This should be moved to the configuration to support different environments.
- { baseUrl: "https://api.turnkey.com" },
+ { baseUrl: config.turnkeyBaseUrl },
Committable suggestion skipped: line range outside the PR's diff.
const humanReadableDate = new Date().toISOString(); | ||
// ID isn't visible by users, but needs to be random enough and valid base64 (for Android) | ||
const userId = Buffer.from(String(Date.now())).toString("base64"); | ||
const authenticatorParams = await turnkeyCreatePasskey({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance user ID generation security
Using Date.now()
for user ID generation is not cryptographically secure and could potentially lead to collisions.
- const userId = Buffer.from(String(Date.now())).toString("base64");
+ const userId = Buffer.from(crypto.randomUUID()).toString("base64");
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const humanReadableDate = new Date().toISOString(); | |
// ID isn't visible by users, but needs to be random enough and valid base64 (for Android) | |
const userId = Buffer.from(String(Date.now())).toString("base64"); | |
const authenticatorParams = await turnkeyCreatePasskey({ | |
const humanReadableDate = new Date().toISOString(); | |
// ID isn't visible by users, but needs to be random enough and valid base64 (for Android) | |
const userId = Buffer.from(crypto.randomUUID()).toString("base64"); | |
const authenticatorParams = await turnkeyCreatePasskey({ |
} catch (err) { | ||
console.log("error creating Xmtp client", err); | ||
setStatusString(""); | ||
handleError( | ||
"Error creating Xmtp client : " + | ||
(err instanceof Error ? err.message : "") + | ||
(typeof err === "string" ? err : "") | ||
); | ||
} finally { | ||
setLoading(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add null check for account in onboardWithPasskey.
The function doesn't check if the account is null before proceeding with XMTP client initialization.
Apply this diff to add proper null checking:
const account = await createPasskey({
passkeyName: inputTextRef.current ?? "",
setStatusString,
setPreviousPasskeyName,
setTurnkeyInfo,
});
+ if (!account) {
+ handleError("Failed to create passkey");
+ return;
+ }
await initXmtpClientFromViemAccount({
account,
address: account.address,
});
Committable suggestion skipped: line range outside the PR's diff.
subOrgId: turnkeyInfo.subOrganizationId, | ||
setStatusString, | ||
}); | ||
setStatusString("Got address and account" + account.address); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove sensitive data from status message.
The status message includes the account address, which should not be displayed in plain text.
Apply this diff to mask the address:
- setStatusString("Got address and account" + account.address);
+ setStatusString("Account successfully created");
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
setStatusString("Got address and account" + account.address); | |
setStatusString("Account successfully created"); |
@@ -85,7 +78,7 @@ func handleV3Message(xmtpClient: XMTP.Client, envelope: XMTP.Xmtp_MessageApi_V1_ | |||
} | |||
// We replaced decodedMessage.senderAddress from inboxId to actual address | |||
// so it appears well in the app until inboxId is a first class citizen | |||
if let senderProfileSocials = await getProfile(account: xmtpClient.address, address: decodedMessage.senderInboxId) { | |||
if let senderProfileSocials = await getInboxIdProfile(account: xmtpClient.address, inboxId: decodedMessage.senderInboxId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asx we come across these methods, they need to be renamed to findThingByInboxId/findThingByAddress
logger.info( | ||
`[Main] Setting navigation ref to ${r ? "not null" : "null"}` | ||
); | ||
// logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove copmment
|
||
const loading = usePasskeyAuthStoreContext((state) => state.loading); | ||
|
||
const statusString = usePasskeyAuthStoreContext( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of selecting all these values, can we just destructure the store context returned here? Can't be hurting perf that much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just checking - is this duped from the NewAccountPasskeyScreen
?
// const { token } = await appCheck.getToken(); | ||
return token; | ||
return "Placeholder"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this still broken for you on android @alexrisch
export async function createSubOrganization( | ||
authenticatorParams: Awaited<ReturnType<typeof createPasskey>> | ||
) { | ||
const res = await createTurnkeySubOrganization(authenticatorParams); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think since sub org is already a turnkey specific implementation detail, we don't need this wrapper
setTurnkeyInfo: (turnkeyInfo: TurnkeyStoreInfo) => void; | ||
}; | ||
|
||
export const loadAccountFromPasskey = async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine if this is just a poc, but is this pattern of passing side effects to what are essentially api calls something we've already established or was that just to move quickly?
The more side effects we have spread out, the harder the logic is to follow.
import { PasskeyStamper } from "@turnkey/react-native-passkey-stamper"; | ||
import { RPID } from "./passkeys.constants"; | ||
|
||
export const turnkeyClientMap: Record<string, TurnkeyClient> = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't both export this object and a getter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good as a poc!
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the high-level release notes:
Release Notes
New Features
Improvements
Bug Fixes
Technical Updates
Platform Support
Security